Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8523 )
Change subject: IMPALA-5931: Generates scan ranges in planner for s3/adls ...................................................................... Patch Set 11: (2 comments) Had a question about the thrift, I will look at the backend side too. http://gerrit.cloudera.org:8080/#/c/8523/11/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/8523/11/common/thrift/PlanNodes.thrift@206 PS11, Line 206: Hdfs I wonder if we should just drop Hdfs from this name given that this is used for other "filesystem" like storage (i.e. S3, ADLS). Ok to ignore if you prefer to keep it for some reason (maybe the reason to keep it is consistency with e.g. HdfsScanNode, which also handles S3/ADLS since they go through the same FileSystem & libhdfs access path). http://gerrit.cloudera.org:8080/#/c/8523/11/common/thrift/PlanNodes.thrift@228 PS11, Line 228: 4: optional THdfsFileSplitGeneratorSpec hdfs_file_split_generator_spec Isn't it that the frontend generates either a list of (concrete) scan ranges, or it just gives a list of FileSplitGeneratorSpec? i.e. I would have expected this to appear as the alternative to Planner.TScanRangeLocationList since the that is the "concrete" scan range list that this thing is going to effectively explode into. i.e. it seems clearer to me to leave TScanRange as the "concrete" range that the coordinator will generate using this new "spec". So what's the reasoning for putting this here instead of higher up in the thrift plan tree? -- To view, visit http://gerrit.cloudera.org:8080/8523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I326065adbb2f7e632814113aae85cb51ca4779a5 Gerrit-Change-Number: 8523 Gerrit-PatchSet: 11 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 07 Mar 2018 22:34:36 +0000 Gerrit-HasComments: Yes
