Zoltan Borok-Nagy 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 5: (10 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& file_path) const; : : static bool IsStreamingIngest(const std::string& file_path) > nit: 'file_path' would be more precise argument name than 'filename' Done 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: // not allowed to see based on its valid write id list. In such cases we need to > A short comment about the purpose of the block would be nice - the whole th Done http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/hdfs-orc-scanner.cc@517 PS3, Line 517: tch* row_batch) { > Can you add some comment about how the case of zeroSlotScan + row validatio Done 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: de(const orc::Type* node) { > column readers and scanner could get this from a common place, e.g. orc-met Done http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc@40 PS3, Line 40: > Is this path possible now that we only do validation if seem necessary? If Currently OrcRowValidator doesn't have access to the scanner to validate this. I added a DCHECK to OrcStructReader::TopLevelReadValueBatch(). http://gerrit.cloudera.org:8080/#/c/15818/3/be/src/exec/orc-column-readers.cc@457 PS3, Line 457: nt_write_ > nit: I would prefer a longer name like write_id_batch Done 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: do t > nit: do Done http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java@463 PS3, Line 463: * One additon to it is to take the visbilityTxnId into consideration. Hence if > plz mention the deviation from the original Hive algorithm like in line 405 Done http://gerrit.cloudera.org:8080/#/c/15818/3/fe/src/main/java/org/apache/impala/util/AcidUtils.java@490 PS3, Line 490: } else if (prev != null && next.maxWriteId == prev.maxWriteId && > Same as in line 463 Added comment http://gerrit.cloudera.org:8080/#/c/15818/3/testdata/workloads/functional-query/queries/QueryTest/acid-negative.test File testdata/workloads/functional-query/queries/QueryTest/acid-negative.test: http://gerrit.cloudera.org:8080/#/c/15818/3/testdata/workloads/functional-query/queries/QueryTest/acid-negative.test@70 PS3, Line 70: refresh acid; : show files in acid; : ---- RESULTS : row_regex:'$NAMENODE/$MAN > use REFRESH acid here Done -- 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: 5 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: Wed, 06 May 2020 11:27:53 +0000 Gerrit-HasComments: Yes
