Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24661 )
Change subject: IMPALA-15247: Unify not-comparable type checks in Type.throwIfNotComparable ...................................................................... Patch Set 4: (3 comments) Read through PS4. Checked that nothing is left asserting the replaced wording (grep over the tree finds none) and that the STRUCT claim in the SetOperationStmt comment holds - castToSetOpCompatibleTypes rejects containsStruct() before that loop. Three notes. http://gerrit.cloudera.org:8080/#/c/24661/4/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/24661/4/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@362 PS4, Line 362: child.getType().throwIfNotComparable("DISTINCT aggregate parameter", child); The old message also named the aggregate itself ("... in aggregate function 'count(distinct v)'"), the new one stops at the parameter. With several distinct aggregates in one query that pointer was doing some work - keep it, or is the parameter enough on its own? http://gerrit.cloudera.org:8080/#/c/24661/4/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/24661/4/fe/src/main/java/org/apache/impala/catalog/Type.java@284 PS4, Line 284: return !isComplexOrVariantType() && !isUuid(); One thing that may be worth a line in the commit message: this branch is not reachable from SQL yet. SlotRef.analyze throws "Reading UUID columns is not yet supported." before any of these checks; casts to and from UUID are not registered (CastExpr line 126); the aggregate, analytic and grouping builtin generators skip UUID (four "TODO: Add UUID builtin support" guards in BuiltinsDb); and the grammar has UUID only as a type name, never as a literal. So I could not find a way to get a UUID-typed expression this far. The message reads as a behaviour change ("UUID is now rejected consistently at these sites") where it is really groundwork for when reading lands - which also explains why no test covers it. http://gerrit.cloudera.org:8080/#/c/24661/4/fe/src/main/java/org/apache/impala/catalog/Type.java@292 PS4, Line 292: */ The single call site that passes null shows the cost of one shared template: SetOperationStmt now produces "UNION, EXCEPT and INTERSECT with type 'VARIANT' is not supported.", which reads as if the operations had a type. The previous wording was "UNION, EXCEPT and INTERSECT are not supported for VARIANT type." The other six sites read well, since they do name an expression. So an overload without an Expr would want its own sentence rather than this one - something like "<what> is not supported for type '<type>'." - which also removes the null argument this thread is about. -- To view, visit http://gerrit.cloudera.org:8080/24661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc2c4579508cf1f8954ac251087c52f2eddf36a Gerrit-Change-Number: 24661 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Tue, 25 Aug 2026 17:14:32 +0000 Gerrit-HasComments: Yes
