fix: resolve null aggregates and broken time_bucket after SQL migration#16
Merged
farhan-syah merged 6 commits intomainfrom Apr 7, 2026
Merged
Conversation
…dedb-sql migration Fixes #15. The aggregate executor receives (op, field) pairs but all three PhysicalTask construction sites were passing the alias instead of the source field name, so the executor could never locate the column and returned null for every MIN/MAX/SUM/AVG result. parse_interval_to_ms only handled compact suffixes ("1h", "15m") and rejected the word-form intervals that DataFusion normalises SQL INTERVAL literals into ("1 hour", "15 minutes"), causing time_bucket() to bucket everything into epoch-zero and return an empty result set. Changes: - sql_plan_convert: extract agg_expr_to_pair() helper that reads the first argument expression (Column name or Wildcard) and use it at all three PhysicalTask construction sites instead of a.alias - aggregate: extend parse_interval_to_ms to accept singular and plural word-form units (hour/hours, minute/minutes, second/seconds, day/days) alongside the existing compact forms; bare numbers are treated as seconds; unrecognised units return 0 instead of silently mis-routing - aggregate: add unit tests covering word-form parsing
Fix rdkafka-sys build failure when compiling with --all-features by adding the missing system libraries required for the Kafka client.
…ES-GCM encrypt/decrypt
…in DEK encryption Update encrypt_dek and decrypt_dek to use the non-deprecated Nonce::from constructor. The decrypt path now performs an explicit TryInto conversion to a [u8; 12] array before constructing the nonce, surfacing a proper error if the slice length is wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #15 — numeric aggregates (MIN/MAX/SUM/AVG) returning null and
time_bucket()returning empty results after the nodedb-sql migration.sql_plan_convertwas passing the display alias (e.g."MIN(elapsed_ms)") as the field name in(op, field)pairs instead of extracting the actual column name from the aggregate args. The executor then failed to find that column in the memtable, leaving accumulators empty → null. Addedagg_expr_to_pair()helper and fixed all 3 call sites. This also fixes the cosmetic column naming issue (count_COUNT(all)→count_all).parse_interval_to_msonly handled single-char suffixes (1h,1m). Word-form intervals like'1 hour'were silently parsed as 0ms, disabling bucketing entirely. Rewrote to split on numeric/unit boundary and accept both short and word forms.Test plan
cargo test -p nodedb-sql -- parse_intervals— unit tests for interval parsing (short + word forms)scripts/test.sh) — added timeseries aggregate coverage: individual and combined MIN/MAX/SUM/AVG, timestamp aggregates, GROUP BY + aggregates, WHERE + aggregates, time_bucket with 1h/1m intervals