Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21039 )
Change subject: IMPALA-12786: Optimize count(*) for JSON scans ...................................................................... Patch Set 10: (5 comments) The patch looks pretty good to me! http://gerrit.cloudera.org:8080/#/c/21039/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21039/10//COMMIT_MSG@53 PS10, Line 53: +--------------+------------+-------+----------+------------+-----------+-------+----------+------------+--------+-------+--------+-----------+ Nice improvement! Do you also get chance to test on nested data set, e.g. tpch_nested? http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/exec/json/json-parser-test.cc File be/src/exec/json/json-parser-test.cc: http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/exec/json/json-parser-test.cc@103 PS10, Line 103: } : else { nit: please put "else" to the line of "}" http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/exec/json/json-parser-test.cc@193 PS10, Line 193: TestSkip(R"({"a":null, "b":[1,true,false]})", TYPE_OBJECT); Can we add more nested cases? E.g. R"({"a":null, "b":{1,true,false}})" // will this fail? R"({"a":null, "b":{"c":"d"}})" R"({"a":null, "b":[{"k1":"v1"}, {"k2":"v2"}]})" I think we need more coverage on the recursive code paths. http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/exec/json/json-parser-test.cc@247 PS10, Line 247: EXPECT_OK(js.Scan(max_rows, &num_rows)); Can we also check row count from this is the same as the final 'row_count'? http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/21039/10/be/src/service/query-options.cc@1259 PS10, Line 1259: query_options->__set_enable_tuple_cache(enable_tuple_cache); missing "break" after this -- To view, visit http://gerrit.cloudera.org:8080/21039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97ff097661c3c577aeafeeb1518408ce7a8a255e Gerrit-Change-Number: 21039 Gerrit-PatchSet: 10 Gerrit-Owner: Zihao Ye <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zihao Ye <[email protected]> Gerrit-Comment-Date: Mon, 29 Apr 2024 09:16:20 +0000 Gerrit-HasComments: Yes
