Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22603 )
Change subject: IMPALA-10349: Support constant folding for non ascii strings ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/22603/9/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/22603/9/common/thrift/Exprs.thrift@121 PS9, Line 121: 1: required binary value; > Before PS11 non-UTF8 default values hit a precondition. Added checks for th Ack http://gerrit.cloudera.org:8080/#/c/22603/11/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java: http://gerrit.cloudera.org:8080/#/c/22603/11/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@71 PS11, Line 71: I nit: "It" ? http://gerrit.cloudera.org:8080/#/c/22603/10/fe/src/main/java/org/apache/impala/util/StringUtils.java File fe/src/main/java/org/apache/impala/util/StringUtils.java: http://gerrit.cloudera.org:8080/#/c/22603/10/fe/src/main/java/org/apache/impala/util/StringUtils.java@37 PS10, Line 37: Preconditions.checkState(false); : return null; > The goal is to throw exception, the return null is just for the compiler. Can we throw the exception explicitly with the string in the message? Using checkState(false) we will only see "IllegalStateException: null" if it fails, which isn't friendly for debug. -- To view, visit http://gerrit.cloudera.org:8080/22603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70663457a0b0a3443e586350f0a5996bb75ba64a Gerrit-Change-Number: 22603 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Sat, 21 Jun 2025 00:49:09 +0000 Gerrit-HasComments: Yes
