Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21684 )
Change subject: IMPALA-13302: Restore registering all conjuncts ...................................................................... Patch Set 5: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/21684/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21684/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2087 PS5, Line 2087: LOG.trace("register eqJoinConjunct: " + Integer.toString(e.getId().asInt())); nit: can we improve this BTW? This log appears twice. We can log the tuple id here. LOG.trace("register eqJoinConjunct {} with tuple id {}", e.getId().asInt(), tupleIds.get(0).asInt()); http://gerrit.cloudera.org:8080/#/c/21684/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2137 PS5, Line 2137: if (e.isBoundByTupleIds(tupleIds) : && !globalState_.assignedConjuncts.contains(e.getId()) : && ((inclOjConjuncts && !e.isConstant()) : || !globalState_.ojClauseByConjunct.containsKey(e.getId()))) { nit: I find it helpful to log the conjuncts that being considered (similar to conjunctAssignmentsDebugString()) and the reasons why a conjunct is added or not. But it seems pretty verbose. -- To view, visit http://gerrit.cloudera.org:8080/21684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5959a3b3e18302e00b1d37e5f50410ebdb224cb0 Gerrit-Change-Number: 21684 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[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-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 03 Sep 2024 12:38:48 +0000 Gerrit-HasComments: Yes
