-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
s3 select improvements #2399
s3 select improvements #2399
Conversation
CI failed -- #2348 |
d9a444f
to
474042e
Compare
474042e
to
34485dc
Compare
|
||
if (TextInputFormat.class.getName().equals(inputFormat)) { | ||
if (!Objects.equals(schema.getProperty(SKIP_HEADER_COUNT_KEY, "0"), "0")) { | ||
// S3 Select supports skipping one line of headers, but it was returning incorrect results for presto-hive-hadoop2/conf/files/test_table_with_header.csv.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shorten to "for gzip files"
// S3 Select supports skipping one line of headers, but it was returning incorrect results for gzip files
Or just the file name test_table_with_header.csv.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to refer to an exact file. I checked that it works correctly some some .gz files, so it looks data-dependent.
I will leave this as-is, and will add a TODO + link to the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave this as-is,
(primarily because i missed this comment earlier)
34485dc
to
812a87a
Compare
S3 Select does not support skipping footer S3 Select supports skipping one line of header, but it not always was returning correct results.
8e67d2e
to
9ca4714
Compare
No description provided.