Zihao Ye has posted comments on this change. ( http://gerrit.cloudera.org:8080/19699 )
Change subject: IMPALA-10798: Prototype a simple JSON File reader ...................................................................... Patch Set 21: (10 comments) http://gerrit.cloudera.org:8080/#/c/19699/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19699/18//COMMIT_MSG@23 PS18, Line 23: > Could you add a section for the current limitations? E.g. Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h File be/src/exec/json-parser.h: http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@45 PS18, Line 45: 'begin' and ' > nit: add quotes on var names, i.e. 'begin' and 'end' Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@49 PS18, Line 49: func > nit: remove "some" Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@68 PS18, Line 68: /// bool AddNumber(int index, const char* str, uint32_t len); > nit: it'd be helpful to give a doc link here. E.g. Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@143 PS18, Line 143: current_field_idx_ = -1; > This is not that readable. Could you add some comments and break them into Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@169 PS18, Line 169: while (!stream_.Eos()) { > What about kParseInsituFlag? Can we use it in our scenario? Unfortunately, we cannot use kParseInsituFlag here because it requires our char stream to provide both input and output abilities simultaneously. Specifically, it needs the ability to write data back from a previous position after reading some data. However, our char stream get the buffer in chunks (see GetNextBuffer), it is difficult to go back the old buffer to write data. http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@230 PS18, Line 230: inline void GetNextBuffer(const char** begin, const char** end) { > nit: This is a bit confusing. It might be better to use DCHECK_EQ(current_f Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json-parser.h@242 PS18, Line 242: /// 2. Call Key() upon encountering a key to find its index of the row in the schema and > nit: could you add a comment before each methods mentioning they are interf Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json/hdfs-json-scanner.cc File be/src/exec/json/hdfs-json-scanner.cc: http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json/hdfs-json-scanner.cc@114 PS18, Line 114: the previous scan ran > nit: "the previous scan range in the same file" might be more clear Done http://gerrit.cloudera.org:8080/#/c/19699/18/be/src/exec/json/hdfs-json-scanner.cc@160 PS18, Line 160: if > nit: add a space after "if" Done -- To view, visit http://gerrit.cloudera.org:8080/19699 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I31309cb8f2d04722a0508b3f9b8f1532ad49a569 Gerrit-Change-Number: 19699 Gerrit-PatchSet: 21 Gerrit-Owner: Zihao Ye <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zihao Ye <[email protected]> Gerrit-Comment-Date: Wed, 19 Jul 2023 08:32:49 +0000 Gerrit-HasComments: Yes
