Skip to content

Move PinotDataType from pinot-common to pinot-spi#18481

Merged
Jackie-Jiang merged 1 commit into
apache:masterfrom
Jackie-Jiang:move-pinot-data-type-to-spi
May 13, 2026
Merged

Move PinotDataType from pinot-common to pinot-spi#18481
Jackie-Jiang merged 1 commit into
apache:masterfrom
Jackie-Jiang:move-pinot-data-type-to-spi

Conversation

@Jackie-Jiang
Copy link
Copy Markdown
Contributor

@Jackie-Jiang Jackie-Jiang commented May 12, 2026

Summary

  • Moves PinotDataType from pinot-common/.../utils/ to pinot-spi/.../utils/, with the matching test (PinotDataTypeTest). The class previously held one pinot-common-specific helper, getPinotDataTypeForExecution(ColumnDataType), which is moved onto ColumnDataType as the instance method toPinotDataType() — leaving PinotDataType free of any pinot-common references.
  • Updates the 22 import declarations across the codebase to the new package, plus two config/checkstyle.xml entries that allow PinotDataType.* static imports.
  • The single existing caller of the moved helper (PostAggregationFunction) is updated to call argumentTypes[i].toPinotDataType().
  • Fixes missing ColumnDataTypePinotDataType mappings that were absent from the original getPinotDataTypeForExecution switch: adds MAP, BOOLEAN_ARRAY, and TIMESTAMP_ARRAY cases. The old switch threw IllegalStateException for these even though they are valid ColumnDataType values.

Motivation

pinot-spi modules (e.g. ColumnReader) currently can't reference PinotDataType because of the dependency direction (pinot-commonpinot-spi). After this move, the SPI's value-type APIs can return a PinotDataType directly — which encodes SV/MV + Java representation in one value — instead of pairing FieldSpec.DataType with a separate isSingleValue() flag. This enables downstream simplifications such as collapsing DataTypeColumnTransformer.isNoOp() to a single equality check against the destination type.

No behavior change apart from the three previously-missing ColumnDataType mappings noted above; otherwise pure code motion plus the rename of one helper.

🤖 Generated with Claude Code

@Jackie-Jiang Jackie-Jiang force-pushed the move-pinot-data-type-to-spi branch from 5f23483 to 8f49f08 Compare May 12, 2026 22:40
@Jackie-Jiang Jackie-Jiang added bug Something is not working as expected refactor Code restructuring without changing behavior labels May 12, 2026
@Jackie-Jiang Jackie-Jiang force-pushed the move-pinot-data-type-to-spi branch from 8f49f08 to 2f73881 Compare May 12, 2026 22:50
This makes PinotDataType reachable from pinot-spi-only modules (ColumnReader
etc.), so the SPI's value-type API can return a PinotDataType that already
encodes SV/MV + Java representation instead of pairing FieldSpec.DataType with
a separate isSingleValue() flag.

The only pinot-common-specific coupling was the static helper
getPinotDataTypeForExecution(ColumnDataType) and its single caller
(PostAggregationFunction). The helper is moved onto ColumnDataType itself as
the instance method toPinotDataType(), so PinotDataType has no dependency on
pinot-common after the move.

22 import declarations across the codebase are updated to the new package; two
checkstyle.xml entries that allow PinotDataType.* static imports are updated to
the new path. PinotDataTypeTest moves alongside.
@Jackie-Jiang Jackie-Jiang force-pushed the move-pinot-data-type-to-spi branch from 2f73881 to 413eab0 Compare May 12, 2026 23:10
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2026

Codecov Report

❌ Patch coverage is 34.37500% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.66%. Comparing base (aec5428) to head (413eab0).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...java/org/apache/pinot/common/utils/DataSchema.java 33.33% 17 Missing and 1 partial ⚠️
.../operator/window/value/LagValueWindowFunction.java 0.00% 1 Missing ⚠️
...operator/window/value/LeadValueWindowFunction.java 0.00% 1 Missing ⚠️
...java/org/apache/pinot/spi/utils/PinotDataType.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18481      +/-   ##
============================================
- Coverage     63.72%   63.66%   -0.07%     
- Complexity     1679     1685       +6     
============================================
  Files          3265     3265              
  Lines        199745   199745              
  Branches      31012    31013       +1     
============================================
- Hits         127296   127175     -121     
- Misses        62296    62432     +136     
+ Partials      10153    10138      -15     
Flag Coverage Δ
custom-integration1 100.00% <ø> (?)
integration 100.00% <ø> (?)
integration1 100.00% <ø> (?)
integration2 0.00% <ø> (?)
java-21 63.66% <34.37%> (-0.07%) ⬇️
temurin 63.66% <34.37%> (-0.07%) ⬇️
unittests 63.66% <34.37%> (-0.07%) ⬇️
unittests1 55.75% <34.37%> (-0.03%) ⬇️
unittests2 34.93% <18.75%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang merged commit c35dee5 into apache:master May 13, 2026
11 checks passed
@Jackie-Jiang Jackie-Jiang deleted the move-pinot-data-type-to-spi branch May 13, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected refactor Code restructuring without changing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants