Skip to content

fix: preserve aggregate ordering when output aliases shadow inputs - #436

Merged
tonyalaribe merged 1 commit into
masterfrom
fix/rum-aggregate-wire-20261025
Oct 26, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
fix/rum-aggregate-wire-20261025

Conversation

@tonyalaribe

@tonyalaribe tonyalaribe commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Aggregate queries such as MAX(last_seen)::TEXT AS last_seen ... ORDER BY MAX(last_seen) DESC fail with a duplicate-field error. Pin the corrected DataFusion fork so aggregate ordering uses the numeric input while direct output-alias ordering keeps text semantics.

All 33 DataFusion patch entries move together to 1f06ab1bd4f27506e0e12d435c36f2673dfbbd98. The lockfile changes only that revision. Fork PR #4 is merged into timefusion-upgrade-55.1 as 08a81e94c5557b01ec5f2cdd64c6770dffad1787; ancestry is verified. The fork also corrects correlated UNNEST SQL generation with existing Snowflake snapshots unchanged.

The real PGWire regression fails on the old dependency and passes on the new one. It checks simple and prepared protocols: aggregate ordering returns 10, 2; direct text-alias ordering returns 2, 10.

Validation:

  • Fork: both required lint suites, all 10,471 extended tests (eight existing skips), and all 106 CLI tests pass.
  • TimeFusion: all eight targeted PGWire compatibility tests pass. make ci-signoff CHECKS="fmt clippy test pg-smoke e2e" passes on exact commit 4d2d5b06125f62ce5f43265867e19b075ad969dc: formatting, lint, 2,892 native tests, 16 doctests, PG smoke, and all 85 end-to-end tests.
  • Native diagnostics: 34 skips and two leaks. E2E: two skips and one flaky deletion-vector fixture, which reports no source files initially, then passes the configured retry and an isolated no-retry rerun. Original failure retained; assertions and retry policy unchanged.
  • Both captured RUM query shapes (regex and ILIKE browser classification) also pass through both protocols with synthetic VALUES input. User order, session counts, affected counts, and session membership are checked. The simple-path shape-cache SUM not supported for Null diagnostic remains nonterminal through fallback for these checked shapes. This is not production-data parity.
  • The Linux amd64 image builds, passes its real PGWire smoke probe, and is published. Image input: 130ad9b6127938600b12fdf8d88fbc48ba1512c52d44e53972da7c6e4a0adbe9. Runtime: ghcr.io/monoscope-tech/timefusion@sha256:fcfb1f25e950e19d000f81bee62e4e77310c7d47b45f4b8404d65525896a0a43.

The observed Monoscope caller remains unattributed. Proxy timing gives candidate external connections, but no exact connection witness or application identity. Deployment and matched production recurrence acceptance remain pending.

@claude

claude Bot commented Oct 26, 2026

Copy link
Copy Markdown

Review: looks good. Nothing blocking.

  • Pin bump: All 33 Cargo.toml entries and the Cargo.lock sources use the same new rev (1f06ab1). I saw no mixed revs and no other lockfile changes. That matches the comment in Cargo.toml about keeping the workspace crates on one source.
  • Test: The test is compact and covers both the extended and simple protocols. Its three orderings pin the intended semantics: numeric MAX(last_seen) gives 10, 2, and the bare output alias gives text order 2, 10. Rows are fixed inline with VALUES, so it needs no table setup and no set_var. This follows the repo's minimal-test guidance.
  • Regression guard: The PR says the test fails on the old pin, which satisfies the repo's bug-fix rule that a guard must be seen to fail.
  • Minor, non-blocking:
    1. Float or other types are not covered, only integers. That is probably fine for the fix.
    2. The Cargo.toml comment above the patch block still describes the older fork feature set. Consider adding a line about this fix.
    3. The fork rev is the only place the fix lives. Please confirm tonyalaribe/datafusion PR feat : object store  #4 is merged and the rev is reachable from a protected branch, so GC or a force-push can't orphan it.
  • Signoff: The PR notes that production image build and deploy signoff are still pending. Per AGENTS.md, don't merge to master (which deploys) until make ci-signoff is attested for the exact tree.

🤖 Generated with Claude Code

@tonyalaribe
tonyalaribe marked this pull request as ready for review October 26, 2026 00:25
@monoscope-bot

monoscope-bot Bot commented Oct 26, 2026

Copy link
Copy Markdown

Monoscope production impact · Coverage unknown

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 4d2d5b06125f62ce5f43265867e19b075ad969dc · advisory

Evidence window (UTC): 2026-10-19 00:25:51.526208228 UTC → 2026-10-26 00:25:51.526208228 UTC

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@tonyalaribe
tonyalaribe merged commit a0da0fc into master Oct 26, 2026
13 checks passed
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.

1 participant