Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle missing written_rows in insert #236

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

hkrutzer
Copy link
Contributor

@hkrutzer hkrutzer commented Feb 3, 2025

This happens in the latest Clickhouse version (25.1)

It should resolve https://github.com/plausible/ecto_ch/actions/runs/13110662749/job/36573653799

lib/ch/query.ex Outdated
%{"written_rows" => written_rows} = Jason.decode!(summary)
with summary <- get_header(headers, "x-clickhouse-summary"),
{:ok, summary} <- Jason.decode(summary),
written_rows when not is_nil(written_rows) <- Map.get(summary, "written_rows") do
Copy link
Contributor

@ruslandoga ruslandoga Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with falls through with the unmatched value (e.g. {:error, Jason.DecodeError.t()}). I think we might need to change the logic a bit to preserve num_rows :: integer | nil

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

num_rows =
  if summary = get_header(headers, "x-clickhouse-summary") do
    summary = Jason.decode!(summary)

    if written_rows = Map.get(summary, "written_rows") do
      String.to_integer(written_rows)
    end
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. What do you think of:

    num_rows =
      with summary <- get_header(headers, "x-clickhouse-summary"),
           summary <- Jason.decode!(summary),
           written_rows when not is_nil(written_rows) <- Map.get(summary, "written_rows") do
        String.to_integer(written_rows)
      end

Copy link
Contributor

@ruslandoga ruslandoga Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would "crash" on missing x-clickhouse-summary (get_header returns nil, with continues, Jason fails on nil) which might be OK. The initial goal was to allow missing x-clickhouse-summary but ensure it's JSON (raise otherwise). Not sure about how important it is now, though. Right now I think we probably shouldn't fail inserts at all on missing num_rows info and document it in ecto_ch that insert_all num_rows is unreliable.

@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 3, 2025

Thank you for the fix!


Strange, I can't reproduce the error locally on current master and ClickHouse local version 25.1.2.3 (official build). ClickHouse stopped returning written_rows in insert into ... select ... inserts.


And #237 passed as well. Ah, the failing CI is actually in ecto_ch! Sorry!


I have verified that the fix works, but I still think we need to be careful with with #236 (comment)

@hkrutzer hkrutzer force-pushed the fix_missing_written_rows branch from 7560ede to c600a85 Compare February 3, 2025 12:38
@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 3, 2025

Thank you! I'm going to cut a release right away!


Published as v0.3.0 since there have been some other changes since v0.2.x.


And I have also published v0.2.10 with just this change: https://diff.hex.pm/diff/ch/0.2.9..0.2.10

@ruslandoga ruslandoga merged commit 220b2a2 into plausible:master Feb 3, 2025
13 checks passed
ruslandoga added a commit that referenced this pull request Feb 3, 2025
@hkrutzer
Copy link
Contributor Author

hkrutzer commented Feb 3, 2025

Thanks, I did end up needing v0.2.10 because ecto_ch doesn't accept v0.3.0. That may not be intentional?

@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 3, 2025

Right, ecto_ch needs to be updated to allow ch 0.3.0: plausible/ecto_ch#221


UPDATE: I've released ecto_ch v0.6.0 which depends on ch v0.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants