Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15818 )
Change subject: IMPALA-9512: Full ACID Milestone 2: Validate rows against the valid write id list ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/acid-metadata-utils.h File be/src/exec/acid-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/acid-metadata-utils.h@39 PS3, Line 39: RangeResponse IsFileRangeValid(const std::string& filename) const; : : static bool IsStreamingIngest(const std::string& filename); nit: 'file_path' would be more precise argument name than 'filename' http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/hdfs-orc-scanner.cc@209 PS3, Line 209: A short comment about the purpose of the block would be nice - the whole thing seems far from intuitive for someone new to ACID. http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/hdfs-orc-scanner.cc@517 PS3, Line 517: row_batches_need_validation_ Can you add some comment about how the case of zeroSlotScan + row validation is handled? http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc@33 PS3, Line 33: CURRENT_TRANSCACTION_TYPE_ID column readers and scanner could get this from a common place, e.g. orc-metadata-utils.h http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc@40 PS3, Line 40: true Is this path possible now that we only do validation if seem necessary? If not, then these could be DCHECKs. http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc@457 PS3, Line 457: wid_batch nit: I would prefer a longer name like write_id_batch http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java@445 PS3, Line 445: does nit: do http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java@463 PS3, Line 463: */ plz mention the deviation from the original Hive algorithm like in line 405 http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java@490 PS3, Line 490: next.statementId == prev.statementId && Same as in line 463 -- To view, visit http://gerrit.cloudera.org:8080/15818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5ed74585a2d73ebbcee763b0545be4412926299d Gerrit-Change-Number: 15818 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 05 May 2020 22:31:06 +0000 Gerrit-HasComments: Yes
