xiangfu0 commented on PR #19101: URL: https://github.com/apache/pinot/pull/19101#issuecomment-5230725515
Follow-up on my review: all four findings are now fixed and merged into this PR (head is now `8b38248f`). | Severity | Finding | Fix | |---|---|---| | Medium | Comparison / `IN` / `IS DISTINCT FROM` guards rejected any non-orderable/non-equatable type (OBJECT, arrays, MAP) with a VARIANT-specific error, narrowing existing behavior | Guards now reject **only** raw `VARIANT`, in both engines: `FilterOperand`, `TransformOperandFactory`, `BinaryOperatorTransformFunction`, `InTransformFunction` | | Medium | VARIANT OVERWRITE-only partial-upsert check only covered listed columns; unlisted columns use `defaultPartialUpsertStrategy` (and a custom `partialUpsertMergerClass`), which could silently apply INCREMENT/APPEND/UNION/IGNORE to a `PVAR` envelope | `TableConfigUtils` now validates the effective strategy for every VARIANT column and rejects a custom merger on VARIANT columns | | Low | Opacity errors said "raw VARIANT" for any non-orderable type | Messages now name the actual type; raw-VARIANT wording + `variantGet` guidance kept only for VARIANT (`VariantTypeValidationVisitor`, `SortOperator`, `SortedMailboxReceiveOperator`, `OrderByComparatorFactory`) | | Low | Planner did not validate `GROUP BY` keys (runtime did) | `VariantTypeValidationVisitor.validateAggregateInputs` now validates `AggregateNode` group keys | Regression tests were added for each. spotless/checkstyle/license are clean on all four affected modules, and every affected unit-test class passes locally on JDK 25 (`VariantTableConfigValidationTest`, `FilterOperandTest`, `VariantTypeValidationVisitorTest`, and the pinot-core comparison/`IN`/`ORDER BY` suites). On the current CI run, the Linter and all VARIANT-exercising tests pass. The three red checks are pre-existing flakes unrelated to these changes and to VARIANT: `FilteredAggregationsTest.testFilterVsCase` (Unit Set 1, same as before this PR) and `testGeneratedQueries`/`testConcurrentQueries` random-query flakes in the two integration sets (no `VariantType*` failures). A re-run should clear them. -- 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]
