Claude finished @paveltiunov's task in 4m 23s —— View job

Review complete — solid fix, 2 medium + 2 low findings (3 posted inline)

• Gather context / fetch diff
• Review schema-compiler template changes
• Review cubesql rewrite + statement changes
• Review tests
• Post review

Overall

Four genuinely independent gaps, each diagnosed to root cause and fixed at the right layer — this is good work. The rewrite-rule change in particular mirrors the existing UDAF idiom (udaf_fun_expr_args / ..._empty_tail) exactly, so the new agg_fun_expr_args rules are structurally consistent with what's already there rather than a one-off. The converter fix (taking e.name(...) before replace_qualified_col_with_flat_name_if_missing) is correct and the zip-over-map pairing is order-safe. Commentary in the diff is unusually good at explaining why, not just what.

Findings

Things I checked and found fine

• Window frames. DataFusion leaves window_frame as None for ranking functions unless the user wrote one, and SqlGenerator::window_frame renders "" for None, so no ROWS/RANGE clause is emitted with ROW_NUMBER/RANK — which matters because T-SQL rejects a frame on a ranking function. The test_distinct_on_cte assertion (ROW_NUMBER() OVER (PARTITION BY) confirms it.
• NtileArgumentCaster scoping. Gated on fun.over.is_some() and on the unqualified name, so a schema-qualified user function named ntile and a non-windowed call are both left alone. The idempotence case is covered by a test, and the BigIntUnsigned guard means a second pass through rewrite_statement can't stack casts.
• Empty-tail rule. wrapper-push-down-aggregate-function-args-empty-tail matches the udaf equivalent including context handling, so a zero-argument aggregate list still pulls up.
• The QuickSight behaviour change is correctly reasoned about in the PR body and matches the cost model's stated preference. One consequence not mentioned there: startsWith/endsWith now arrive as segment member expressions rather than native filters, which can affect pre-aggregation matching for those BI queries — member-expression filters generally match fewer pre-aggregations than native ones. Probably acceptable given the correctness win (ranking a row-capped result is simply wrong), but worth a conscious decision rather than a side effect.
• Template name coverage. functions/APPROXPERCENTILECONT is not in the base templates, and the test that exercises it injects the template explicitly — correctly modelling "only dialects with an approximate percentile define it".

I did not build or run the Rust suite here (the PR reports 864 passing, fmt and clippy clean); findings above are from reading the diff.

Claude finished @paveltiunov's task in 1m 25s —— View job

Re-review of 45a20a5 — finding 1 addressed and resolved; 2 prior threads still open

• Fetch new commits since last review
• Review 45a20a5 (APPROX_PERCENTILE_CONT arity)
• Re-check prior unresolved threads
• Resolve stale threads / post update

What changed

One new commit since the previous review: 45a20a5 test(cubesql): pin APPROX_PERCENTILE_CONT to its two-argument form — 30 lines, test-only, no production change.

Finding 1 (Presto APPROX_PERCENTILE arity) — resolved, and my premise was wrong

I claimed DataFusion's approx_percentile_cont takes an optional third centroid argument that would mis-map onto Presto's approx_percentile(x, w, percentage). It doesn't here — the weighted form is a separate function, approx_percentile_cont_with_weight(x, w, percentile), which lines up with Presto's three-arg form rather than clashing with it. The new test_wrapper_approx_percentile_cont_is_binary pins that: a three-argument call is rejected at planning with does not accept 3 function arguments, before rewriting ever sees it, so args_concat cannot render an arity the template doesn't mean.

I verified the second half of the claim directly — APPROXPERCENTILECONTWITHWEIGHT is defined by no dialect (grep over packages/cubejs-schema-compiler/src and rust/cubesql/src finds only APPROXPERCENTILECONT), so the weighted variant has no push-down path at all. Keeping args_concat is the right call: pinning to args[0]/args[1] would have silently truncated a future third argument instead of failing.

The test also guards the right thing going forward — a DataFusion bump that relaxes the arity fails this test rather than shipping a wrong query, and the doc comment tells whoever hits it why. Thread resolved.

Prior threads that still apply (not reposted)

Re-affirmed both threads still apply; left unresolved rather than replying.

Verdict

The new commit is a clean, well-reasoned resolution of the one finding it targets, and it corrects a misreading on my side rather than papering over it. Remaining items are one medium (dialect template honesty) and one low (test assertion precision); neither blocks the correctness of the push-down work.

I did not build or run the Rust suite in this job — the DataFusion sources aren't vendored in this checkout, so the arity claim is verified from the test's own assertion and the author's reported green run, not by execution.

Claude finished @paveltiunov's task in 1m 4s —— View job

Re-review of f919be9 — finding 3 resolved, finding 2 half-addressed (ClickHouse still open)

• Fetch new commits since last review
• Review f919be9 (MongoBI templates + test tightening)
• Re-check prior unresolved threads
• Resolve stale threads / post update

What changed

One new commit: f919be9 fix(schema-compiler): drop window function templates for MongoBI — two files, both items I raised.

MongoBI (MongoBiQuery.ts:25-42). New sqlTemplates() override deleting all eleven window function templates, including the pre-existing LAG/LEAD. Correct: MongoBiQuery extends MysqlQuery, MySQL 8 does have window functions so keeping them on the parent is right, and the BI Connector's 5.7-era dialect is the exception that has to opt out. Verified test/unit/mongobi-query.test.ts asserts nothing about templates, so nothing breaks there. delete templates.functions.X matches the MssqlQuery idiom already in the codebase, and the comment says why rather than restating the code.

Test tightening (test_wrapper.rs:4133-4157). plan.is_err()→expect_err(...)pluserror.to_string().contains("Can't detect Cube query"). I confirmed that string is the real error (converter.rs:1766, :1988`), so the assertion pins the actual failure path rather than any failure. The rewritten doc comment also now states what happens — a hard planning failure, not a fallback — which is the user-visible behaviour on a dialect without the template. Thread 3 resolved.

Prior thread still open

Verdict

Both items in this commit are done properly rather than minimally — the MongoBI fix cleans up the pre-existing LAG/LEAD leak while it's in there, and the test change pins a real error string instead of just narrowing the assertion shape. One medium remains (ClickHouse CUME_DIST), and it is a one-line delete in the same style if you agree with the reading.

I did not build or run the suites in this job (no npx tsc permission here, Rust sources not built); findings are from reading the diff plus targeted greps.

Codecov Report

❌ Patch coverage is 97.96954% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.57%. Comparing base (5bc6308) to head (5e6acd8).
⚠️ Report is 11 commits behind head on master.

@@             Coverage Diff             @@
##           master   #11684       +/-   ##
===========================================
+ Coverage   58.77%   84.57%   +25.80%     
===========================================
  Files         237      261       +24     
  Lines       18932    85747    +66815     
  Branches     3841        0     -3841     
===========================================
+ Hits        11128    72524    +61396     
- Misses       7243    13223     +5980     
+ Partials      561        0      -561     

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CI note — tests (snowflake-export-bucket-s3-via-storage-integration-iam-roles, true) failed, and it isn't this PR's.

What failed. Job 98952556227, reporting 112 of 139 tests failed. That count is the tell: the suite never ran. From its log:

(HTTP code 500) server error - Get "https://registry-1.docker.io/v2/":
  context deadline exceeded (Client.Timeout exceeded while awaiting headers)
● Test suite failed to run
TypeError: Cannot read properties of undefined (reading 'release')

Docker Hub was unreachable, so the container the driver suite needs never started, every test in the file errored at setup, and teardown then tripped over the undefined container. No test body executed.

Why it isn't this PR's. The diff touches cubesql rewrite rules, cubesql statement rewriting, and four schema-compiler dialect adapters — BaseQuery, MssqlQuery, MongoBiQuery, PrestodbQuery. It contains nothing about Snowflake, S3 export buckets, storage-integration IAM roles, or the container images this job pulls. A real regression from these changes would surface as a handful of snapshot diffs, not as the whole file failing to start.

No re-run spent, and none needed. That job ran against 45a20a5, which f919be9 has since superseded, and the Drivers tests workflow already has a fresh run in flight for the current head (run 33201371878). Re-running a superseded commit would prove nothing the live run won't. If the same job fails on f919be9 for the same registry reason I'll re-run it there once; if it fails for any other reason I'll treat it as this PR's and root-cause it.

Everything else on the current head is green, including Unit (Rewrite Engine) (the cubesql suite carrying this PR's new tests), Check fmt/clippy, lint, CodeQL and both native Linux builds.

Generated by Claude Code

Second CI note — Build windows-2022 24.x with Python fallback also failed, and this one isn't this PR's either. Different check, different cause from the Snowflake note above.

No test failed. Job 98948955568 ends:

A worker process has failed to exit gracefully and has been force exited.
This is likely caused by tests leaking due to improper teardown.
Test Suites: 1 failed, 1 skipped, 2 passed, 3 of 4 total
Tests:       11 skipped, 13 passed, 24 total
Force exiting Jest: ...

13 passed, 11 skipped, 0 failed. The suite is marked failed purely because Jest force-exited on a leaked worker handle. Every SQL API query in the log planned and returned successfully (SQL API Query Planning Success, Load Request Success, 60–72 ms each) — including the push-down paths this PR changes.

Why it isn't this PR's.

1. It ran against bdb345c, two commits behind the current f919be9.
2. This build matrix fails intermittently on master too, in a different cell each time — e.g. run 33003719609 on master (2aba3ac, "Push UNION down to the data source") failed on Build Linux GNU 24.x aarch64-unknown-linux-gnu with Python 3.9. A code regression wouldn't wander between matrix cells like that.
3. The diff is planning logic only — rewrite rules, a statement visitor, SQL templates. It opens no sockets, spawns no threads and holds no handles, so there is nothing in it that could keep a Jest worker alive.

No re-run spent. The same job is already running against the current head in run 33201371894, where Check fmt/clippy, Unit (Rewrite Engine) and two of the build matrix cells have already gone green. If Windows fails there for this same force-exit reason I'll re-run it once; if it fails for any other reason I'll treat it as this PR's and root-cause it.

Generated by Claude Code

Third CI note — Upload merged coverage to Codecov. Re-run spent, failed identically. Not this PR's, but unlike the other two this one is reproducible and has a real root cause worth fixing separately.

What happens. Both the original (job 98956846228) and the re-run (job 98962521842) die at the same step with the same error, after successfully pulling most artifacts:

##[error]Unable to download artifact(s): Unable to download and extract artifact:
Artifact download failed after 5 retries.

Likely cause. upload-coverage in .github/workflows/push.yml downloads every artifact in the run — there is no pattern filter:

      - name: Download all coverage artifacts
        uses: actions/download-artifact@v4
        with:
          path: all-coverage

but the very next step consumes only lcov files:

          find all-coverage -name '*.lcov' -exec cat {} + > merged-coverage.info

So alongside coverage-unit and coverage-integration-*, it also pulls artifacts it then throws away — including cubestored-x86_64-unknown-linux-gnu-release and native-linux-x64-glibc-*.node, which are large binaries, plus cypress-screenshots-docker-dev-*. That is the most plausible reason a download stalls through 5 retries, and it explains why a re-run reproduces it rather than clearing it.

Proposed patch — narrowing the download to what the merge step actually reads:

       - name: Download all coverage artifacts
         uses: actions/download-artifact@v4
         with:
           path: all-coverage
+          pattern: coverage-*

I have deliberately not pushed this. It is a repo-wide CI change with no relation to SQL push down, and folding it into this PR would widen it well past its subject. Happy to open it as its own PR if a maintainer wants it.

Why this doesn't reflect on the change. The diff produces no artifacts and touches no CI config. Coverage itself reached Codecov regardless — its report is on this PR at 97.20% patch coverage, project coverage up. And every functional job on f919be9 is green: Unit (Rewrite Engine), unit (24.x, 3.13), lint, Check fmt/clippy, build-cubestore, both integration-smoke jobs, and all 19 integration dialects — athena, trino, prestodb, mongobi, mssql and clickhouse among them, which are precisely the adapters this PR changes. 29 of 30 jobs in the run passed.

One caveat on my own evidence: I could not establish a master baseline for this job, because upload-coverage does not appear in the recent master runs of push.yml I sampled. So I am relying on the root cause above rather than on a red-on-master comparison.

That is my one re-run spent, so I will not re-run it a third time.

Generated by Claude Code

Claude finished @paveltiunov's task in 1m 27s —— View job

Re-review of 090c493 (NTILE revert) — clean and complete; 1 low finding posted

• Fetch new commit since last review
• Review the revert (template, router, test, statement.rs)
• Re-check prior unresolved threads
• Post update

What changed

One new commit: 090c493 Revert "fix(cubesql): cast NTILE's argument to plan it" — the NtileArgumentCaster statement rewrite is gone; the NTILE SQL template stays with a comment pointing at CORE-831.

The revert is complete. I checked for stragglers rather than trusting the revert:

• grep -rn "Ntile\|NTILE\|ntile" rust/cubesql/cubesql/src finds only the two explanatory comments and compile/test/mod.rs:701, which registers functions/NTILE in the test tenant context — correct, since it mirrors the base templates that still define it.
• router.rs drops both the import and the rewrite_statement call; no half-removal.
• statement.rs is exactly two hunks (the impl and its tests) with no import churn — the caster used ast::DataType qualified, so nothing is left unused. Expr, Function, FunctionArg, FunctionArgExpr, FunctionArguments and Value all still have other users in the file, so the import list is untouched and correct.

The judgment is right. Casting NTILE(4) to BIGINT UNSIGNED in a statement visitor was patching a signature bug one layer below where it lives, and it was the only part of this PR carrying that shape. Keeping the template while removing the workaround is the right split: the template is independently correct, so CORE-831 lands as a signature relaxation in the fork with no change here. Worth noting the template isn't strictly dead in the meantime either — a user who writes their own NTILE(CAST(4 AS BIGINT UNSIGNED)) satisfies the signature and pushes down through it today.

Removing the case also keeps test_wrapper_built_in_window_functions honest: it now asserts push-down only for what actually pushes down, rather than needing a rewrite to prop up one entry.

Finding

Prior threads

All three of my earlier threads are now resolved — nothing re-affirmed, nothing new on BaseQuery.js or PrestodbQuery.ts, which this commit doesn't touch beyond the one comment.

For the record on the CUME_DIST thread: your reasoning for not deleting it convinced me, and the reason is worth keeping visible. Deleting a template doesn't degrade to a slower fallback — for a query without a LIMIT it degrades to a silently wrong answer, since the window then runs in DataFusion over a row-capped scan. A loud syntax error on an old ClickHouse server is the better failure of the two, and settling the "floor to oldest supported version vs. track current" policy across adapters is genuinely a separate change.

Verdict

Approved-shaped from my side. The revert narrows the PR to the gaps it actually set out to fix, removes the one piece that was working around a problem it didn't own, and documents the deferral at both the template and the test. One low, entirely optional, test-hygiene note.

I did not build or run the Rust suite in this job; the revert analysis is from reading the diff plus targeted greps for dangling references.

Claude finished @paveltiunov's task in 57s —— View job

Re-review of 5f2c232 — the NTILE pin is right; no blocking findings, all prior threads resolved

• Fetch new commit since last review
• Review the NTILE pin test
• Re-check prior unresolved threads
• Resolve stale threads / post update

What changed

One new commit, test-only: 5f2c232 test(cubesql): pin NTILE's planning failure instead of commenting it out (+40/−6 in test_wrapper.rs).

The commented-out ("NTILE(4)", "NTILE(4)") case in test_wrapper_built_in_window_functions becomes a two-line pointer, and the explanation moves into a new executable test, test_wrapper_ntile_does_not_plan (test_wrapper.rs:4110), which asserts the current failure with expect_err(...) plus a message check. This is the shape I suggested and it's applied cleanly:

• The query shape matches its neighbour — same grouped subselect and OVER (ORDER BY notes) as the case list it will rejoin — so when it goes red, the case that gets added back is known to be equivalent to the one being pinned, not a differently-shaped approximation.
• The doc comment states the exit condition, naming both the file location and the exact tuple to add. That's the part a comment-only marker couldn't do.
• The pointer left behind in the case list (test_wrapper.rs:3876) keeps the two halves discoverable from either direction, without duplicating the rationale.
• Imports check out — convert_sql_to_cube_query, get_test_tenant_ctx and get_test_session are already in the file's use list (:18-20), so no import churn.

Worth noting the assertion also does the other job I raised: Exact([UInt64]) appears in the message only if the NTILE signature is still the strict one, so a dialect deleting the template or a rename of the SQL entry no longer passes unnoticed.

One note, no change requested

assert!(error.to_string().contains("Exact([UInt64])")) matches the Debug rendering of DataFusion's TypeSignature, not a stable API string. A DataFusion bump that reformats the coercion error would fail this test without CORE-831 having landed — which contradicts the doc comment's "when this test starts failing, that fix has landed". The failure is still loud and the fix obvious from the message diff, so this is a footnote rather than a finding; if you want it airtight, a sentence in the doc comment that a message-format change is the other way it can go red would cover it.

Prior threads

All four of my earlier threads are resolved — the PrestodbQuery arity thread, the BaseQuery dialect-template thread, the is_err() tightening thread, and the NTILE thread this commit closes. Nothing re-affirmed, nothing new posted.

Verdict

The PR now pins every deliberate gap with a test rather than a comment: test_wrapper_approx_percentile_cont_is_binary for the arity, test_wrapper_ntile_does_not_plan for the signature, and test_wrapper_multi_arg_aggregate_function_without_template for the missing-template path. That's a consistent standard across all three deferrals, and it's the difference between a known limitation and a forgotten one. Nothing blocking from my side.

I did not build or run the Rust suite in this job — the DataFusion sources aren't vendored in this checkout, so the Exact([UInt64]) message is verified from the test's own assertion and the author's reported green run, not by execution.

Took the footnote from the 5f2c232 re-review even though it was marked no-change-requested — it identified a real inaccuracy in my own doc comment, and the cost of leaving it is that someone acts on wrong instructions.

The comment asserted a biconditional that isn't true: "when this test starts failing, CORE-831 landed." As noted, Exact([UInt64]) is the Debug rendering of a TypeSignature, not a stable API string, so a DataFusion bump that reformats the coercion error goes red without the signature having been relaxed — and whoever hit that would follow the comment straight into deleting a test that is still doing its job.

5e6acd8 documents both paths and, more usefully, how to tell them apart from the error text itself:

One other thing can turn it red. Exact([UInt64]) is the Debug rendering of a TypeSignature, not a stable string, so a DataFusion bump that reformats the coercion error fails this assertion while NTILE still does not plan. The error text says which happened: if it no longer mentions a coercion at all, the signature was relaxed and the case can move back; if it still refuses the argument in different words, only the assertion needs updating.

I kept the assertion matching on Exact([UInt64]) rather than loosening it to something like "coercion". Precision is the point of the pin — a looser match would keep passing through exactly the signature change it exists to detect. Better to have a test that occasionally goes red for a documented second reason than one that stays green through the event it was written for.

Generated by Claude Code

Claude finished @paveltiunov's task in 36s —— View job

Re-review of 5e6acd8 — docs-only, closes my last footnote; no findings, all threads resolved

• Fetch new commit since last review
• Review the doc comment change
• Re-check prior review threads
• Post update

What changed

One new commit, comment-only: 5e6acd8 docs(cubesql): note the other way the NTILE pin can go red — +7 lines of doc comment on test_wrapper_ntile_does_not_plan (test_wrapper.rs:4113-4118), no executable change.

This is the footnote I raised last round, taken further than I asked. I suggested a sentence saying a message-format change is the other way the test can go red; the commit adds that and a disambiguation rule for whoever hits it:

if it no longer mentions a coercion at all, the signature was relaxed and the case can move back; if it still refuses the argument in different words, only the assertion needs updating.

That's the part that matters. The risk with the original comment wasn't the ambiguity itself — it was that the comment gave an unconditional instruction ("delete it and add the case back") that a DataFusion reformat would trigger falsely. A reader following it on the wrong kind of red would have re-added NTILE(4) to test_wrapper_built_in_window_functions and turned a precise failure into a vague one. The added paragraph gates that instruction on evidence the reader can check from the panic output.

One refinement, no change requested

The new text says "the error text says which happened", but the two reds surface differently:

• Signature relaxed — NTILE(4) plans, so .expect_err(...) panics with NTILE should not plan until CORE-831 lands. There's no error text at all; the panic message is the signal.
• Message reformatted — planning still fails, so the assert! fires and prints unexpected error: <new text>, which is where the "does it still mention a coercion" test applies.

So the reader distinguishes them by which of the two panics fires, before ever reading error text — arguably clearer than the comment lets on. Not worth another commit; the outcome of following the comment as written is correct either way.

Prior threads

All four of my threads are resolved — PrestodbQuery arity, BaseQuery dialect templates, the is_err() tightening, and the NTILE pin. Nothing re-affirmed, nothing new posted.

Verdict

Nothing blocking, and nothing outstanding from my side. The PR ended up with each of its three deliberate gaps pinned by an executable test rather than a comment — test_wrapper_approx_percentile_cont_is_binary, test_wrapper_ntile_does_not_plan, test_wrapper_multi_arg_aggregate_function_without_template — and this commit makes the last of those safe to act on. The CUME_DIST / dialect-versioning question is the one open item, and it's correctly scoped out as a cross-adapter policy call rather than folded in here.

I did not build or run the Rust suite in this job; the change is a doc comment, verified by reading it against the test body directly beneath it.

Fourth CI note — integration-smoke (24.x, 3.13, false) failed on the current head 5e6acd8. Not this PR's.

All 5 tests died in the same beforeAll, at job 98974079153:

testcontainers [ERROR] [d8da5fa7df25] Health check failed: unhealthy
testcontainers [ERROR] [d8da5fa7df25] Container failed to be ready

● multidb › query
    Health check failed: unhealthy
      53 |   beforeAll(async () => {
      54 |     db = await PostgresDBRunner.startContainer({});
    > 55 |     db2 = await MysqlDBRunner.startContainer({});

The mysql:8.0 container started but never went healthy inside the wait timeout, so beforeAll threw and every test in smoke-multidb reported that same message. afterAll then tripped over the undefined birdbox (Cannot read properties of undefined (reading 'stop')). No test body executed.

The obsolete snapshot is a symptom, not a cause. › 1 snapshot obsolete • multidb query: query 1 looks alarming next to a SQL-generation PR, so I checked it specifically: the snapshot is orphaned because no test ran to produce it this round, not because generated SQL changed.

Why it isn't this PR's. The sibling variant integration-smoke (24.x, 3.13, true) passed on this exact commit, in this exact run — same code, same suite, differing only in the Tesseract planner flag. Identical code cannot pass in one and fail in the other on a SQL-generation regression; it can when one Docker container comes up healthy and the other doesn't. And nothing in this diff influences container health.

No re-run. My one re-run was spent earlier on Upload merged coverage to Codecov, where it failed identically and is documented above. I am not re-running a second job.

For context, this is the fifth distinct environmental failure on this PR today — a Docker Hub registry timeout, a Jest worker force-exit on Windows, the coverage artifact download, an MSSQL sa login failure, and now this MySQL health check. Every one landed on infrastructure rather than on the diff, and each failing check that has a comparison point has a green counterpart on the same or a later commit. Worth a glance at CI health independently of this PR.

Generated by Claude Code