pinot: work around STRING MIN/MAX limitation in queries.sql (apache/pinot#19603) - #2125
Open
KazukiKandaKK wants to merge 2 commits into
Open
KazukiKandaKK wants to merge 2 commits into
KazukiKandaKK wants to merge 2 commits into
Conversation
…d stop/start race pinot/ had three independent bugs, found while running the real ~74GB ClickBench hits.tsv dataset on a c6a.4xlarge (32 GiB RAM) EC2 instance: 1. Cold-cycle data loss (root cause of 0/43 query failures) Pinot's benchmark.sh has effectively run with BENCH_DURABLE=yes since the BENCH_RESTARTABLE -> BENCH_DURABLE rename in commit b282aa4 -- the wrong setting for a system whose loaded state lives only in process memory. Commit b422b2d ("Remove unnecessary BENCH_DURABLE=yes") later deleted the explicit `export BENCH_DURABLE=yes` line, but that's a red herring: BENCH_DURABLE already defaults to `yes` in lib/benchmark-common.sh, so deleting the redundant explicit line changed nothing -- Pinot's effective value was `yes` before that commit and stayed `yes` after it. Pinot's own `-dataDir` persistence flag does not work as documented either (verified locally: throws IllegalStateException on the second QuickStart start, Pinot 1.5.1). Since a full CSV reload takes ~78 minutes and can't run before every one of 43 queries, `load` now sets BENCH_DURABLE=no and re-pushes the existing on-disk segment .tar.gz files via Pinot's official Tar Push API (docs.pinot.apache.org/.../segment-upload) instead of re-parsing hits.tsv from scratch on every cold cycle. 2. Silent partial segment generation under memory pressure pinot-admin.sh defaults to `-Xms4G` with no `-Xmx`, so it falls back to the JVM's default heap ceiling (~25% of physical RAM). On the real 74GB/100-split hits.tsv this OOMed mid-job around segment 60/100 (OutOfMemoryError in SegmentDictionaryCreator) -- but LaunchDataIngestionJob still exited 0 and bench_load's >5GB data-size guard didn't catch it either, since 60 segments already exceeded 5GB. `load` now sets `JAVA_OPTS="-Xms4G -Xmx20G"` and explicitly compares the number of generated segment tars against the number of input splits, failing loudly on any mismatch instead of silently proceeding to the query phase with incomplete data. 3. stop/start race corrupting the next cold cycle `stop` only did `pkill` + a fixed `sleep 2`, but a broker can keep answering for 7-8s after SIGTERM while the JVM shutdown hook is still running, and the embedded ZooKeeper's own port teardown lags even further behind that. The next `start` then either treats the still-alive old broker as "already up" (skipping the real restart) or fails to bind ZooKeeper's port. `stop` now polls until the pinot-admin JVM process itself is fully gone (matched via `pgrep -f 'org.apache.pinot.tools.admin.PinotAdministrator'`) instead of guessing from a single component's HTTP port. `check` similarly no longer treats broker-responsive as "fully started": QuickStart only registers its clean-shutdown hook, and finishes bootstrapping, after printing "Quick start setup complete" to pinot.log; check now greps for that line (scoped to the current invocation via a recorded log offset) in addition to the broker health check, so `stop` is never sent while Pinot is still mid-startup. Verified end-to-end on EC2 (c6a.4xlarge, real 74GB hits.tsv, 100 segments): 37/43 queries now pass across all 3 cold-cycle trials (up from 0/43). The remaining 6 failures are a separate, pre-existing Apache Pinot limitation (standard MIN()/MAX() rejects STRING columns; reported upstream at apache/pinot#19603) unrelated to this benchmark-driver fix.
…inot#19603) Depends on ClickHouse#2124 (cold-cycle fix) -- without it, pinot/ fails all 43 queries before this even matters. With that fix applied, 6 of the 43 queries still fail. All 6 use MIN/MAX/extract/DATE_TRUNC on EventTime, a STRING column with a SIMPLE_DATE_FORMAT dateTimeFieldSpec. Standard SQL MIN/MAX in Pinot always compiles to a numeric-only aggregation function regardless of the column's declared type, confirmed in AggregationFunctionType (pinot-segment-spi): // TODO: min/max only supports NUMERIC in Pinot, where Calcite // supports COMPARABLE_ORDERED MIN("min", SqlTypeName.DOUBLE, SqlTypeName.DOUBLE), MAX("max", SqlTypeName.DOUBLE, SqlTypeName.DOUBLE), NonScanBasedAggregationOperator's fast aggregation path then calls toDouble() unconditionally on the dictionary value, which throws NumberFormatException: For input string: "2013-07-01". Pinot already ships a fix for this (apache/pinot#16980, merged 2025-10-10): an AggregateFunctionRewriteOptimizer that rewrites MIN/MAX on a STRING column to MINSTRING/MAXSTRING. A follow-up (apache/pinot#17058, merged 2025-10-30) gated the rewrite behind a query option: its diff adds `if (Boolean.parseBoolean(options.get(QueryOptionKey.AUTO_REWRITE_AGGREGATION_TYPE))) { useRuleSet.add(...AGGREGATE_FUNCTION_REWRITE); }`, so autoRewriteAggregationType defaults to off when unset. Confirmed Pinot 1.5.1 (used here) postdates both merges. Filed apache/pinot#19603 upstream, since neither the option nor MINSTRING/MAXSTRING is mentioned on the Query Options doc page and the error message gives no hint they exist. This PR turns the option on for every ClickBench query: - pinot/query: adds autoRewriteAggregationType=true - pinot/queries.sql: EventTime is used directly in extract(minute FROM EventTime) and DATE_TRUNC('minute', EventTime) in queries 19 and 43. Those aren't MIN/MAX, so the option doesn't touch them; they fail for the same underlying reason (Calcite treats the STRING column as non-temporal). Wrapped both in CAST(EventTime AS TIMESTAMP), matching the existing pattern in this benchmark's sail/sail-partitioned queries.sql, which already do the exact same cast for these same two queries. Verified locally (Docker, Pinot 1.5.1, 3-row STRING table) that OPTION(autoRewriteAggregationType=true) alone fixes the MIN/MAX case, and on EC2 that all 6 previously-failing queries now return non-null results with this change plus the cold-cycle fix from ClickHouse#2124.
KazukiKandaKK
requested a deployment
to
benchmark-approval
September 20, 2026 13:48 — with 
GitHub Actions
Waiting
GitHub Actions
Waiting
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.
Depends on the cold-cycle fix in #2124 — without it,
pinot/fails all 43 queries before this even matters.With that fix applied, 6 of the 43 queries still fail. All 6 use
MIN/MAX/extract/DATE_TRUNConEventTime, a STRING column with aSIMPLE_DATE_FORMATdateTimeFieldSpec. Standard SQLMIN/MAXin Pinot always compiles to a numeric-only aggregation function regardless of the column's declared type — confirmed inAggregationFunctionType(pinot-segment-spi), which has:NonScanBasedAggregationOperator's fast aggregation path then callstoDouble()unconditionally on the dictionary value, which throwsNumberFormatException: For input string: "2013-07-01".Pinot already ships a fix for this (apache/pinot#16980, merged 2025-10-10): an
AggregateFunctionRewriteOptimizer(added in that PR's diff,pinot-query-planner) that rewritesMIN/MAXon a STRING column toMINSTRING/MAXSTRING. A follow-up (apache/pinot#17058, merged 2025-10-30) gated the rewrite behind a query option: its diff addsif (Boolean.parseBoolean(options.get(QueryOptionKey.AUTO_REWRITE_AGGREGATION_TYPE))) { useRuleSet.add(...AGGREGATE_FUNCTION_REWRITE); }, soautoRewriteAggregationTypedefaults to off when unset. Confirmed Pinot 1.5.1 (used here) postdates both merges. Filed apache/pinot#19603 upstream (open, no response yet as of this PR), since neither the option norMINSTRING/MAXSTRINGis mentioned on the Query Options doc page and the error message gives no hint they exist.This PR just turns the option on for every ClickBench query:
pinot/query:option(timeoutMs=300000)→option(timeoutMs=300000,autoRewriteAggregationType=true)pinot/queries.sql:EventTimeis used directly inextract(minute FROM EventTime)andDATE_TRUNC('minute', EventTime)in queries 19 and 43. Those aren'tMIN/MAX, soautoRewriteAggregationTypedoesn't touch them; they fail the same way for the same underlying reason (Calcite treats the STRING column as non-temporal). Wrapped both inCAST(EventTime AS TIMESTAMP), matching the existing pattern in this benchmark'ssail/sail-partitionedqueries.sql, which already do the exact same cast for these same two queries.Verified locally (Docker, Pinot 1.5.1, 3-row STRING table) that
OPTION(autoRewriteAggregationType=true)alone fixes theMIN/MAXcase, and on EC2 that all 6 previously-failing queries now return non-null results with this change plus the cold-cycle fix from #2124.