Dan Hecht has posted comments on this change. Change subject: IMPALA-2835: introduce PARQUET_FALLBACK_SCHEMA_RESOLUTION query option ......................................................................
Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/2384/7/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 2031: TParquetFallbackSchemaResolution::POSITION); move this DCHECK to L2065. no reason to have a separate if-stmt for it when it can be incorporated into the code control flow. also, consider getting rid of 'resolve by_name' variable. http://gerrit.cloudera.org:8080/#/c/2384/7/be/src/service/query-options.cc File be/src/service/query-options.cc: Line 371: "0" Why allow the numerical enum value? (Especially given that the enum is not exposed)? I see in other options we sometimes allow it and other times don't, so I guess I'm okay either way but curious about the reasoning. Line 379: position in other statuses above, we use CAPS for the option name, no quotes, and also put the numerical value in parenthesis. would be nice to be consistent. (Though I think the parenthesis notation for the number is kinda confusing) -- To view, visit http://gerrit.cloudera.org:8080/2384 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0c715ea23792b2a6872610839a40532aabbb5a6 Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
