Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19380 )
Change subject: IMPALA-10893: Use old schema during iceberg time travel. ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/19380/2/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/19380/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1309 PS2, Line 1309: timeTravelTable.readSchema(); > I did that initially, but then I split it out. Splitting out to a separate function is nice, but I would call it in the constructor. Deferred construction puts in extra layer of complexity to the usage of the object (we can forget to call this in the future). In my opinion, it is worth it if - the deferred part is computation-heavy - and the object is "sane" and usable without it in some cases. http://gerrit.cloudera.org:8080/#/c/19380/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/19380/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@453 PS4, Line 453: Path resolvedPath = analyzer_.resolvePathWithMasking(slotRef.getRawPath(), : PathType.SLOT_REF, null); We could add a similar overload for these guys too. http://gerrit.cloudera.org:8080/#/c/19380/4/fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java: http://gerrit.cloudera.org:8080/#/c/19380/4/fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java@197 PS4, Line 197: class ForwardingFeIcebergTable implements FeIcebergTable { : private final FeIcebergTable base; I agree 100% with the pattern, I just wish there was a better way to do it. C++ private inheritance comes close, but still need to "reimplement"/change/extend the API with forwarding to the base class. -- To view, visit http://gerrit.cloudera.org:8080/19380 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7cbef6e20bbb567e517744fb1f34d880970399ab Gerrit-Change-Number: 19380 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Jan 2023 08:46:21 +0000 Gerrit-HasComments: Yes
