Repository navigation
Prepare Yardstick for DuckDB 2.0 - #52
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8a8d32819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #if YARDSTICK_PARSER_INSTANCE_API | ||
| // GetParserOptions is private in 2.0. Snapshot the same public settings for | ||
| // deferred binding; NativeYardstickParseScope disables override recursion. | ||
| auto options = ParserOptions::Builtin(); |
There was a problem hiding this comment.
When a DuckDB 2.0 session enables another parser option such as order_by_non_integer_literal, this manually constructed snapshot leaves that option at ParserOptions::Builtin()'s default. Queries that go through yardstick(...) or deferred SELECT binding are therefore reparsed differently from the originating session and can be rejected even though the same SQL is accepted directly. Copy every context-dependent parser option here, including order_by_non_integer_literal, rather than only the five currently listed.
Useful? React with 👍 / 👎.
| wrap_decorated_scalar_recomputation(&eval_sql) | ||
| } else { | ||
| format!("MAX({eval_sql})") | ||
| format!("ANY_VALUE({eval_sql})") |
There was a problem hiding this comment.
For an ungrouped non-decomposable measure whose AT (SET ...) expression depends on the outer row—for example, AGGREGATE(revenue) AT (SET year = year - 1) over several years—eval_sql can yield different values for different source rows even though original_dim_cols is empty. ANY_VALUE then chooses an arbitrary one, making the result scan-order dependent and changing the previous deterministic MAX behavior. The LIST compatibility fix should not use ANY_VALUE unless the generated values are proven identical; use a type-aware collapse or explicitly validate the single-value invariant.
Useful? React with 👍 / 👎.
1 participant
Yardstick's previous 2.0 target followed a moving release branch and no longer compiled against the final engine. Pin the submodule and release CI to DuckDB 2.0.0 at
d54cbbe3df17e8e5ce68312f483b4155836112aa, with matching pinned extension CI tooling, while retaining the legacy 1.5.5 job and advisory main canary.Adapt parser construction, expression parsing, grammar transformer arena ownership, and parse-tree traversal to the final APIs. Preserve session grammar and parser settings during deferred binding, plus the caller's
order_by_non_integer_literalbinder setting on the wrapper's private execution connection. Replace scalar-contextMAXwrappers withLIST_MAX(LIST(...))to avoid DuckDB 2.0's LIST subquery binding failure while preserving deterministic maximum semantics when correlated values differ. This workaround buffers the candidate values. Update native suites for the removed parser toggle and add session-setting, correlated-context, and LIST regressions.Validated locally on macOS ARM64 using checksum-verified official DuckDB release libraries: