You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/06/03 04:24:58 UTC
[PR] Deduplicate convertToColumnDataType between RelToPlanNodeConverter and PRelToPlanNodeConverter [pinot]
yashmayya opened a new pull request, #18661:
URL: https://github.com/apache/pinot/pull/18661
## Summary
`pinot-query-planner` carried two byte-for-byte identical copies of the `SqlTypeName -> DataSchema.ColumnDataType` mapping switch (and its private `resolveDecimal` helper):
- `org.apache.pinot.query.planner.logical.RelToPlanNodeConverter#convertToColumnDataType`
- `org.apache.pinot.query.planner.physical.v2.PRelToPlanNodeConverter#convertToColumnDataType`
This is a duplicated source of truth that risks the two type tables silently drifting apart (it nearly did during the Calcite 1.42 unsigned-integer-type work). Both classes live in the same module and `PRelToPlanNodeConverter` already imports from the `logical` package, so there is no module-boundary reason for the parallel implementation.
## Change
`PRelToPlanNodeConverter.toDataSchema` now delegates directly to `RelToPlanNodeConverter.convertToColumnDataType`, mirroring how `PinotEvaluateLiteralRule` already calls it. The duplicated `convertToColumnDataType` / `resolveDecimal` methods — which had no external callers — are removed, along with the imports and `LOGGER` field that only they used. `RelToPlanNodeConverter.convertToColumnDataType` is now the single source of truth.
This is a pure, behavior-preserving refactor: the removed methods were identical to the delegation target.
## Testing
- Full `pinot-query-planner` test suite passes (1248/1248), including `RelToPlanNodeConverterTest`, `WorkerManagerTest` (which exercises the v2 physical-planner path through `PRelToPlanNodeConverter`), and the `ResourceBasedQueryPlansTest` plan snapshots (no plan-output drift).
- `spotless`, `checkstyle`, and `license` checks pass.
--
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] Deduplicate convertToColumnDataType between RelToPlanNodeConverter and PRelToPlanNodeConverter [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18661:
URL: https://github.com/apache/pinot/pull/18661#issuecomment-4609318075
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18661?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 56.87%. Comparing base ([`f6b930b`](https://app.codecov.io/gh/apache/pinot/commit/f6b930b4de86cde50a03713dd6c611032c2929b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`14fff1d`](https://app.codecov.io/gh/apache/pinot/commit/14fff1dfe6f42e14a2ffb902ade35be619a2d152?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 25 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (f6b930b) and HEAD (14fff1d). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (f6b930b) | HEAD (14fff1d) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18661 +/- ##
============================================
- Coverage 64.45% 56.87% -7.59%
+ Complexity 1282 1 -1281
============================================
Files 3352 2579 -773
Lines 207171 149922 -57249
Branches 32348 24224 -8124
============================================
- Hits 133534 85261 -48273
+ Misses 62910 57454 -5456
+ Partials 10727 7207 -3520
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.87% <100.00%> (-7.59%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.87% <100.00%> (-7.59%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.87% <100.00%> (-7.59%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.87% <100.00%> (+0.05%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18661/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/18661?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] Deduplicate convertToColumnDataType between RelToPlanNodeConverter and PRelToPlanNodeConverter [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18661:
URL: https://github.com/apache/pinot/pull/18661#issuecomment-4966078527
Compared the removed `PRelToPlanNodeConverter.convertToColumnDataType` against `RelToPlanNodeConverter.convertToColumnDataType` (line 1131) and the mapping is identical right down to `resolveDecimal` and the UBIGINT rejection, so pointing `toDataSchema` at it is behavior preserving. The target is already `public static` in the same module and this class was importing `RexExpression`/`RexExpressionUtils` from the `logical` package anyway, and the dropped imports plus the `LOGGER` field were only used by the deleted methods, so nothing is left dangling. Removing `PRelToPlanNodeConverterTest` is fine too, since it was the only external caller of the deleted method and `RelToPlanNodeConverterTest` already exercises the same unsigned matrix (UTINYINT/USMALLINT to INT, UINTEGER to LONG, UBIGINT throws, plus the array variants) directly on the method the v2 path now calls. No objections from me.
--
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