You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/09/02 23:55:28 UTC
[PR] Resolve absent MAP/OPEN_STRUCT keys to a shared default-column-style NullDataSource [pinot]
Jackie-Jiang opened a new pull request, #19439:
URL: https://github.com/apache/pinot/pull/19439
## Summary
`NullDataSource` is now a single class in the `datasource` package of `pinot-segment-local`, shared by MAP and OPEN_STRUCT, and built exactly like a default column (a schema column missing from the segment): it reuses `DefaultNullValueVirtualColumnProvider` and only adds an all-null `NullValueVectorReader`. Every absent key therefore gets a single-entry dictionary holding the field's default null value, a `ConstantSortedIndexReader` (or the constant MV forward and inverted readers), and `ColumnMetadataImpl` with cardinality 1, sorted for single-value, and the default as min/max, so with null handling on the key reads as NULL and with it off as the default.
Previously MAP and OPEN_STRUCT each had their own all-null source. The MAP one was hardcoded as an `INT` column with `numDocs = 0`, so an absent key's type disagreed with the map's value type, and both served hardcoded dimension defaults from a raw forward index, ignoring `BOOLEAN` / `TIMESTAMP` defaults (`0`, not `MIN_VALUE`), METRIC defaults and custom `defaultNullValue`s.
### Absent keys are resolved by the data sources
`OpenStructDataSource.getDataSource(key)` and `MapDataSource.getDataSource(key)` never return `null` anymore: a definitively absent key resolves to the `NullDataSource` inside the implementation. The new default method `OpenStructDataSource.getKeyFieldSpec(key)` holds the rule the contract already documented (declared child spec, else a single-value STRING), and `ImmutableOpenStructDataSource` uses it for sparse keys too. Both SPIs are implemented only in this repo.
- `ProjectionBlock`, `ItemTransformFunction` and `MapFilterOperator` lose their "synthesize when `null`" fallbacks. `MapFilterOperator.buildAbsentKeyFilterOperator` and its raw-value `matchesDefaultNullValue` switch are gone: the one-entry dictionary makes every EQ / NOT_EQ / IN / NOT_IN / RANGE evaluator fold to always-true or always-false, and `getLeafFilterOperator` already handles always-true under null handling through the null vector, so the regular per-key path yields the same match-all / match-none operators.
- `BaseMapDataSource.getDataSource` no longer catches, logs and swallows exceptions while building a key's data source, which turned a broken per-key index into "absent key"; it propagates them.
- `ItemTransformFunction` uses the per-key null vector for an absent MAP key, which is exact, and keeps falling back to the map column's bitmap for present keys, which carry no per-key null information.
### Also
`BaseConstantValueVirtualColumnProvider.buildMetadata` now sets the total number of entries to the doc count, so `getNumValues()` is correct for every constant-value virtual column instead of `0`.
### Behavior notes
- Absent OPEN_STRUCT and MAP keys are dictionary-encoded like default columns, so `item()` over them now reports a dictionary.
- A key excluded by the sparse manifest on a segment that also has a sparse JSON index goes through `JsonMatchFilterOperator` instead of the per-key fold: same result, different EXPLAIN.
- No `MapIndexReader` in this repo returns `null` from `getIndexes`, so the MAP absent-key path is only reached through the contract branch; the type fix matters for implementations that do.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Resolve absent MAP/OPEN_STRUCT keys to a shared default-column-style NullDataSource [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19439:
URL: https://github.com/apache/pinot/pull/19439#issuecomment-5547700508
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1032
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Resolve absent MAP/OPEN_STRUCT keys to a shared default-column-style NullDataSource [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19439:
URL: https://github.com/apache/pinot/pull/19439
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Resolve absent MAP/OPEN_STRUCT keys to a shared default-column-style NullDataSource [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19439:
URL: https://github.com/apache/pinot/pull/19439#issuecomment-5518452884
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19439?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`d3e53e2`](https://app.codecov.io/gh/apache/pinot/commit/d3e53e202107605fb68f6e10aef6f55e25152d29?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2c8b8f4`](https://app.codecov.io/gh/apache/pinot/commit/2c8b8f4bcaedd7907ed7b0061dfdafca939f3788?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19439 +/- ##
==============================================
+ Coverage 67.62% 100.00% +32.37%
+ Complexity 1430 6 -1424
==============================================
Files 3488 3 -3485
Lines 224390 6 -224384
Branches 35423 0 -35423
==============================================
- Hits 151753 6 -151747
+ Misses 60607 0 -60607
+ Partials 12030 0 -12030
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.37%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.37%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19439/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19439?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Resolve absent MAP/OPEN_STRUCT keys to a shared default-column-style NullDataSource [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19439:
URL: https://github.com/apache/pinot/pull/19439#issuecomment-5522122486
Addressed in the latest push (single amended commit `2168a68`).
**Critical**
1. Nullable `OpenStructDataSource` contract: not taken. The interface is in no Apache release (added by #18368, first ships in 1.6.0) and has no implementation outside this repo, so there is no third-party implementation to stay compatible with. Keeping the method nullable would keep the three call-site fallbacks this PR removes.
2. Sparse JSON fast path vs custom default null values: fixed. `trySparseJsonIndex` now refuses the key's actual default null value instead of only the STRING sentinel, so a manifest-absent key with a custom default takes the per-key fold and agrees with fully materialized segments. Covered by a new case in `testSparseJsonIndexRefusals`.
3. Real MAP absent-key path: added `BaseMapDataSourceTest`, which drives `BaseMapDataSource.getDataSource` with a `MapIndexReader` that reports the key absent and asserts the value-typed all-null source. An end-to-end query cannot reach this path in this repo: neither `MapIndexReader` implementation ever reports a key absent, and `MapFieldTypeTest` already covers absent keys through that path on both engines.
**Major**
- Constant folding: the previous fold also built the all-null source before evaluating, and the `IS_NULL` bitmap is a single run container, so nothing new is materialized per query. Left as is.
- `ImmutableOpenStructDataSourceTest` line 159: fixed, it now asserts the STRING fallback type.
- Renamed `getKeyFieldSpec` to `getValueFieldSpec`.
- Trailer removed, and the root-cause rationale moved into the commit message.
**Minor**
- Deprecated wrappers: the removed classes are internal to `pinot-segment-local` and one of them is unreleased, so no shims.
- Thread safety documented on `NullDataSource`.
- `FieldSpec.DataType` matches the existing style of the touched test files.
Also fixed the two CI failures this surfaced: the mocked `QueryContext` in `MapFilterOperatorOpenStructTest` now serves `getOrComputeSharedValue`, which the dictionary-based IN evaluator uses, and `DefaultNullValueVirtualColumnProviderTest` expects the entry count that `buildMetadata` now reports.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org