yashmayya commented on PR #18848: URL: https://github.com/apache/pinot/pull/18848#issuecomment-4928944097
Good catch, @xiangfu0 — this is a real no-false-negative violation. I dug into the exact semantics and it's slightly broader than multi-key, so I fixed it for both arities and added tests. Details: **What actually diverges.** I traced all three comparisons: - **Multi-key** hash join keys are `org.apache.pinot.core.data.table.Key` in an `ObjectLookupTable` (HashMap) → `Arrays.equals`/`Arrays.hashCode` → `Double.equals`/`doubleToLongBits`, which **canonicalizes** every NaN payload. - **Single-key** FLOAT/DOUBLE hash join uses `Double2ObjectOpenHashMap`/`Float2ObjectOpenHashMap` (`DoubleLookupTable`/`FloatLookupTable`), i.e. fastutil **raw bits** (`doubleToRawLongBits`) — it distinguishes NaN payloads. - The **leaf exact-`IN`** set (`DoubleOpenHashSet`/`FloatOpenHashSet`) compares by raw bits too, **but** the build literals are canonicalized on the way in: `getLiteralExpression` → `getLiteralString` (`Double.toString`/`Float.toString`) → `"NaN"` → `parseDouble`. So the set only ever holds the canonical NaN and then compares the probe raw. So exact-`IN` is a *canonical-build ∪ raw-probe* mix that matches **neither** join. It can drop a joinable probe row on **both** paths: - multi-key: a build NaN matches any probe NaN payload in the join, but the leaf (canonical-only set) drops a non-canonical probe NaN; - single-key: build and probe sharing the same *non-canonical* NaN match in the join (raw==raw), but the leaf canonicalizes the build literal and drops the probe. The **bloom tier is unaffected**: it adds the raw build value (no stringification) and `BloomFilterIdSet` uses raw bits for both add and contains — matching the single-key hash join. (Multi-key never blooms.) **Fix** (just pushed, `38e9b94c5a`): abandon the exact-`IN` tier whenever a FLOAT/DOUBLE build key contains `NaN` — covers single- and multi-key. The bloom tier is untouched, so an AUTO single-key FLOAT/DOUBLE with a large NaN-containing build still reduces via bloom; only the exact-`IN` path (which can't represent NaN faithfully) drops out. I went this way rather than your option (b) because "canonicalizing the exact-IN semantics" means changing the shared leaf `IN`-on-double evaluator (raw-bit membership), which affects every `IN` query and is really a separate, pre-existing quirk in Pinot's `IN`/double semantics. **Tests:** - Unit (`ServerPlanRequestUtilsTest`): single-key DOUBLE and FLOAT exact-`IN` with NaN → abandoned; a **composite key with a non-canonical NaN payload** (`0x7ff8000000000001`) → abandoned; a non-NaN FLOAT/DOUBLE key still gets an exact `IN` (guard is targeted); and an AUTO single-key NaN DOUBLE above `maxInSize` still emits a bloom. - Integration (`RuntimeFilterJoinIntegrationTest.testCompositeNaNKeyMatchesBaseline`): a composite `(DOUBLE NaN, INT)` self-join, results identical across off/in/bloom/auto. On the regression you asked for: the deterministic one is the unit test at the attach layer (I can construct an arbitrary non-canonical NaN payload directly in the build rows). An end-to-end distinct-payload test isn't reliable — a non-canonical NaN has to survive Avro encoding and segment ingestion on a noDictionary column, and ingestion may canonicalize it, so an end-to-end test with the canonical `Double.NaN` wouldn't actually exercise the divergence (it passes with or without the fix). Two orthogonal things I noticed while tracing, both pre-existing and out of scope for this PR (happy to file issues if useful): (1) the single-key FLOAT/DOUBLE hash join distinguishes NaN payloads (raw bits) while the multi-key one canonicalizes them — an engine inconsistency in its own right; (2) the shared `attachDynamicFilter` is also used by the SEMI dynamic-broadcast path, whose leaf `IN` has the same string canonicalization — but SEMI *replaces* the join with the `IN` rather than adding to it, so its NaN semantics need their own analysis before concluding anything is wrong there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
