Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15259 )
Change subject: IMPALA-4080 [part 2]: Invoke codegen from scan's plan node instead of exec node ...................................................................... Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/15259/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15259/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-4080 [part 2]: Invoke codegen from scan's plan node instead of > weird wrapping Done http://gerrit.cloudera.org:8080/#/c/15259/1//COMMIT_MSG@10 PS1, Line 10: This patch moves the code responsible for invoking codegen for > Is this sentence right? It seems like the code hasn't moved, but the plan n Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-avro-scanner.h File be/src/exec/hdfs-avro-scanner.h: http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-avro-scanner.h@98 PS1, Line 98: static Status Codegen( > Does dropping WARN_UNUSED_RESULT has a reason? The same happens in many oth Tim pointed out earlier that we no longer need this since clang checks already catch dropped Status. So cleaning up code where i can http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-avro-scanner.h@211 PS1, Line 211: static Status CodegenDecodeAvroData(const HdfsScanPlanNode* node, RuntimeState* state, > Same as line 98. see previous reply http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-avro-scanner.h@237 PS1, Line 237: & > Why change this to a pointer? As far as I could see from the implementation Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.h@107 PS1, Line 107: /// Returns index into materialized_slots with 'path'. Returns SKIP_COLUMN if > Consider removing this or making it a constant. I don't think the generalit Removed it, here and in the scan exec node http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.h@133 PS1, Line 133: > Init to -1 just so any bugs are more obvious? Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.h@139 PS1, Line 139: // > FWIW I personally find this pattern of typedefing containers to make the co Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.h@165 PS1, Line 165: or all Hdfs s > Maybe rename to something like 'scanned_file_formats_' to be more explicit. Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc@a390 PS1, Line 390: > That was a weird place to put this... moved this to HdfsScanNodeBase::IssueInitialScanRanges() instead http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc@229 PS1, Line 229: const THdfsFileSplit& split = scan_range_param.scan_range.hdfs_file_split; > I guess this is not exactly equivalent to the previous logic if different i Yup, all of this is in anticipation of doing this once per fragment, so it needs to make sure it includes all file types from ranges assigned to all instances. I've switched it to only get the ranges from the current instance ctx and added a TODO for now. http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc@291 PS1, Line 291: AddBytesReadCounters(); > Maybe remove this comment? Not sure it's useful. Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc@409 PS1, Line 409: } > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/exec/hdfs-scan-node-base.cc@948 PS1, Line 948: eriali > nit: no std:: needed Done http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/15259/1/be/src/runtime/query-state.h@249 PS1, Line 249: static const int DEFAULT_BATCH_SIZE = 1024; > Nit: return const & since they shouldn't be null removed this part for now -- To view, visit http://gerrit.cloudera.org:8080/15259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02c6183271b3731aa0d3cae2426be7269e462967 Gerrit-Change-Number: 15259 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 24 Feb 2020 17:57:41 +0000 Gerrit-HasComments: Yes
