You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2018/03/07 01:48:39 UTC
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Zach Amsden has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9525
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
This is conflated a bit with the support work required to get
MySQL to run in the mini-cluster to support HMS, as Postgresql
simply rejects these table creations outright.
The basic idea is to specialize the class on whether row/tuple
delimiters are actually used, as the use case for sequence and
text files is quite different. Unfortunately, that came out a
bit more ugly than I would have liked, as the class template
parameter infected all definitions.
Testing: Created a zero delimited table as described in the JIRA,
ran select * from tab_separated on the table.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M bin/create-test-configuration.sh
M bin/impala-config.sh
M buildall.sh
M fe/pom.xml
M testdata/bin/run-sentry-service.sh
12 files changed, 136 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/1
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2159/
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 4
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 23:17:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 6:
I think so; I'll let it run through GVO first.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Mar 2018 19:32:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc
File be/src/exec/delimited-text-parser-test.cc:
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc@160
PS2, Line 160: TupleDelimitedTextParser nul_delim_parser(NUM_COLS, 0, is_materialized_col, NUL_DELIM);
> should we also test nul tuple delim with a column delim? if not, why is tha
Will add a test for this.
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@22
PS2, Line 22: #include <type_traits>
Will delete this
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@41
PS2, Line 41: thme
> them
Done
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@55
PS2, Line 55: char field_delim_ = '\0', char collection_item_delim = '^',
: char escape_char = '\0');
> do these default argument values still have special meanings? can the delim
The only place these have interesting meanings is the unit test. I don't think the defaults are very useful, but I also don't think removing them and adding a bunch of extra unexplained fields to the unit test is helpful either.
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@171
PS2, Line 171: bool IsFieldOrCollectionItemDelimiter(char c) {
> comment for this -- it's surprisingly non-trivial to understand what this i
Will add a comment.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:17:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9525
to look at the new patch set (#3).
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 167 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/3
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 3
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Mar 2018 23:19:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 6: Code-Review+2
> I think so; I'll let it run through GVO first.
Yes, of course, by merge I meant merge via GVO. You'll need to carry the +2 forward unless you made a non-trivial change (which looks like it was).
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Mar 2018 21:45:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 4
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:35:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9525
to look at the new patch set (#4).
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 167 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/4
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 4
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 6:
Zach, can this be merged?
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Mar 2018 16:09:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Reviewed-on: http://gerrit.cloudera.org:8080/9525
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 169 insertions(+), 84 deletions(-)
Approvals:
Dan Hecht: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 7
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2186/
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Mar 2018 19:31:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9525
to look at the new patch set (#5).
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 169 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/5
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 5
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163: tuple_delim_ || (tuple_delim_
> I don't follow that. When DELIMITED_TUPLES is false, the comment says tuple
I agree the logic here is kind of broken; if tuple_delim_ == field_delim_, this isn't correct for !DELIMITED_TUPLES; however, in practice tuple_delim_ can not be equal to field_delim_ except in the unit test; this is prohibited by the frontend.
To compound the matter, the only place this matters, in hdfs-sequence-scanner.cc never checks this value, as by general design, the sequence scanner always knows where the tuple ends and never has to deal with unfinished tuples.
This makes me a bit loathe to make the logic here more complicated.
A good compromise might be to make this also include DELIMITED_TUPLES as a conjunct, and then add a DCHECK in HasUnfinishedTuples() that DELIMITED_TUPLES is true.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:34:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@174
PS3, Line 174: field_delim_ != tuple_delim_
> It is not allowed to have tables created like this by the frontend. It is
hmm, okay. this seems a bit weird to me to handle cases (and have to think about them and have tests for them) that aren't valid. Besides, for the test that do this, are we sure they actually exercise the interesting (i.e. valid) cases? That said, let's leave it alone for now, this can of worms doesn't seem worth opening further right now.
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc
File be/src/exec/delimited-text-parser.cc:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc@182
PS3, Line 182: if (DELIMITED_TUPLES) unfinished_tuple_ = false;
> Nope; I was just eliding all references to unfinished_tuple_ in the class v
I don't think we need to code it explicitly. My comment about DCHECK is to help the code reader, not the compiler -- to document the fact that the entire code block from 170-189 is not relevant when DELIMITED_TUPLES.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 3
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 17:06:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2159/
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 4
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:36:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@29
PS1, Line 29: delimited_tuples
comment on what that is.
it'd be nice if we used all caps for template args more consistently, but I see we already have template args in this class that don't do that, so okay to ignore.
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@51
PS1, Line 51: tuple_delim
what should be passed in here when !delimited_tuples? let's at least note in the comment above that this is ignored when !delimited_tuples
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@96
PS1, Line 96: /// This function is disabled for non-sequence file parsing.
: template <bool process_escapes, bool enabled=true>
: typename std::enable_if<!delimited_tuples && enabled, Status>::type
why do we need that?
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@222
PS1, Line 222: tuple_delim_
how about saying that's valid only when delimited_tuples is true?
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@55
PS1, Line 55: b
why isn't that delimited_tuples? is it something different
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163: tuple_delim_ || (tuple_delim_
do those need to be guarded?
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@235
PS1, Line 235: enabled
what's that about?
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 21:00:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9525
to look at the new patch set (#2).
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 147 insertions(+), 78 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/2
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163: tuple_delim_ || (tuple_delim_
> I agree the logic here is kind of broken; if tuple_delim_ == field_delim_,
Including DELIMITED_TUPLES as a conjunct here makes sense to me and also adding the dcheck to HasUnfinishedTuples().
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 22:36:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 1:
Did you mean to post a new patch?
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 23:11:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@174
PS3, Line 174: field_delim_ != tuple_delim_
is it allowed to have field_delim_ same as tuple_delim_? what's the behavior in that case? looking at the callsites, I don't see how we ever add new columns in that case. and do we test it?
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@175
PS3, Line 175: )
this would be a little easier to read without the extraneous parens around the whole thing (I kept looking to see which clauses were grouped only to find it covers the entire thing).
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc
File be/src/exec/delimited-text-parser.cc:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc@182
PS3, Line 182: if (DELIMITED_TUPLES) unfinished_tuple_ = false;
can we even get here if !DELIMITED_TUPLES? If not, maybe DCHECK(DELIMINTED_TUPLES) on line 170 (and revert this if-stmt).
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 3
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:16:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@51
PS1, Line 51: tuple_delim
> what should be passed in here when !delimited_tuples? let's at least note
Done
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@96
PS1, Line 96: /// This function is disabled for non-sequence file parsing.
: template <bool process_escapes, bool enabled=true>
: typename std::enable_if<!delimited_tuples && enabled, Status>::type
> why do we need that?
Not necessary. Initially I thought there was more split functionality in this class than there actually is and I wanted to conditionally get rid of functions that were invalid based on delimited_tuples. I think a DCHECK would be more than sufficient.
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@222
PS1, Line 222: tuple_delim_
> how about saying that's valid only when delimited_tuples is true?
Done
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@55
PS1, Line 55: b
> why isn't that delimited_tuples? is it something different
will fix
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163: tuple_delim_ || (tuple_delim_
> do those need to be guarded?
Subtly, no. Any delimiter of '\0' will not be included in the xmm_delim_search field, so the corresponding bit will never be set.
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@235
PS1, Line 235: enabled
> what's that about?
An extra fake template parameter defaulting to true is used in the class definition to eliminating this function by utilizing SFINAE because it isn't legal to use fail substitution on definitions only depending on a class template parameter.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 21:19:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9525
to look at the new patch set (#6).
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
IMPALA-6389: Make '\0' delimited text files work
Initially I didn't want to fully implement this, as the metadata
for these tables can't even be fully stored in Postgres; however
after digging into some older documentation, it appears that the
ASCII NUL character actually has been used as a field separator
in various vendors CSV implementation.
Therefore, this patch attempts to make things as non-broken as
possible and allows \0 as a field or tuple delimiter. Collection
column delimiters are not allowed to be \0, as they genuinly may
not exist and we don't want to force special escaping on an
arbitrary character. Note that the field delimiter must be distinct
from the tuple delimiter when they both exist; if it is not, the
effect will be that there is no field delimiter (this is actually
possible with single column tables).
Testing: Created a zero delimited table as described in the JIRA,
using MySQL backed Hive metastore; ran select * from tab_separated
on the table, updated the unit test.
Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/delimited-text-parser.inline.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
8 files changed, 169 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/9525/6
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 6
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 4:
Sigh. Release build failed. Fixed now.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 4
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 01:59:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@174
PS3, Line 174: field_delim_ != tuple_delim_
> hmm, okay. this seems a bit weird to me to handle cases (and have to think
I'm not super happy about this, as I found the can of worms during testing. The tests now exercise pretty much all of the cases, both valid and invalid. I did not extend the tests to validate sequence file parsing (we have no tests for this).
Mostly, I am concerned with just not crashing and having well defined behavior in all cases, no matter what, even if we get nonsensical metadata for the file.
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc
File be/src/exec/delimited-text-parser.cc:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc@182
PS3, Line 182: if (DELIMITED_TUPLES) unfinished_tuple_ = false;
> I don't think we need to code it explicitly. My comment about DCHECK is to
Done
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 3
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:54:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc
File be/src/exec/delimited-text-parser-test.cc:
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc@160
PS2, Line 160: TupleDelimitedTextParser nul_delim_parser(NUM_COLS, 0, is_materialized_col, NUL_DELIM);
should we also test nul tuple delim with a column delim? if not, why is that case not interesting?
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@41
PS2, Line 41: thme
them
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@55
PS2, Line 55: char field_delim_ = '\0', char collection_item_delim = '^',
: char escape_char = '\0');
do these default argument values still have special meanings? can the delims be equal or do they need to be distinct from each other? please update the comment above to indicate anything interesting.
does it still make sense to have these default arguments?
http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@171
PS2, Line 171: bool IsFieldOrCollectionItemDelimiter(char c) {
comment for this -- it's surprisingly non-trivial to understand what this is (e.g. what does the field_delim_ == tuple_delim_ case mean? are they allowed to be equal?)
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:
http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163: tuple_delim_ || (tuple_delim_
> Subtly, no. Any delimiter of '\0' will not be included in the xmm_delim_se
I don't follow that. When DELIMITED_TUPLES is false, the comment says tuple_delim_ can be set to anything.
Also, isn't the point to handle '\0' as a delimiter, so why is that not included in the search field?
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 00:41:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@174
PS3, Line 174: field_delim_ != tuple_delim_
> is it allowed to have field_delim_ same as tuple_delim_? what's the behavio
It is not allowed to have tables created like this by the frontend. It is possible, although unlikely that tables like this already exist. I encountered this behavior in the test, when the default arguments for the constructor created such a parser, so it is tested. I thought the sanest behavior was to only recognize a field delimiter when it was not equal to a tuple delimiter, and tuple delimiters were enabled. Before this, the behavior was nuts, it would not recognize tuples, and instead treat the entire row as an infinite set of columns.
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@175
PS3, Line 175: )
> this would be a little easier to read without the extraneous parens around
Oh yes, it is just a bunch of OR clauses; we can remove these.
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc
File be/src/exec/delimited-text-parser.cc:
http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc@182
PS3, Line 182: if (DELIMITED_TUPLES) unfinished_tuple_ = false;
> can we even get here if !DELIMITED_TUPLES? If not, maybe DCHECK(DELIMINTED
Nope; I was just eliding all references to unfinished_tuple_ in the class variant that had DELIMITED_TUPLES as false. I would hope in this case the compiler can help out by proving that new_tuple is always false, but we can help it out by coding it explicitly if you think that would be better.
--
To view, visit http://gerrit.cloudera.org:8080/9525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 3
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 17:00:19 +0000
Gerrit-HasComments: Yes