Skip to content

Add TIME_MICROS support to GenericParquetReaders - #487

Merged
rdblue merged 3 commits into
apache:masterfrom
ashleycoxley:time_read_fix
Sep 23, 2019
Merged

Add TIME_MICROS support to GenericParquetReaders#487
rdblue merged 3 commits into
apache:masterfrom
ashleycoxley:time_read_fix

Conversation

@ashleycoxley

@ashleycoxley ashleycoxley commented Sep 20, 2019

Copy link
Copy Markdown
Contributor

Closes #482

Comment thread data/src/main/java/org/apache/iceberg/data/parquet/GenericParquetReaders.java Outdated
import java.time.OffsetDateTime;
import java.time.ZoneOffset;

import java.time.*;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid wildcard imports because it isn't clear where symbols are coming from and there is potential for collision. Could you roll back this change? (As well as the additional newline, we don't use breaks in imports either.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

@rdblue

rdblue commented Sep 20, 2019

Copy link
Copy Markdown
Contributor

Thanks for working on this, @ashleycoxley!

@rdblue

rdblue commented Sep 21, 2019

Copy link
Copy Markdown
Contributor

Looks like there are checkstyle violations:

[ant:checkstyle] [ERROR] /home/travis/build/apache/incubator-iceberg/data/src/main/java/org/apache/iceberg/data/parquet/GenericParquetReaders.java:26: Extra separation in import group before 'java.time.Instant' [ImportOrder]
[ant:checkstyle] [ERROR] /home/travis/build/apache/incubator-iceberg/data/src/main/java/org/apache/iceberg/data/parquet/GenericParquetReaders.java:379:44: '{' at column 44 should have line break after. [LeftCurly]
[ant:checkstyle] [ERROR] /home/travis/build/apache/incubator-iceberg/data/src/main/java/org/apache/iceberg/data/parquet/GenericParquetReaders.java:379:103: '}' at column 103 should be alone on a line. [RightCurly]

@rdblue
rdblue merged commit 8f8605e into apache:master Sep 23, 2019
@rdblue

rdblue commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

Merged! Thanks for the quick updates, @ashleycoxley!

If you'd like to open a PR for TIME_MILLIS as well, I can help review it.

@aokolnychyi

Copy link
Copy Markdown
Contributor

I've created an issue to support TIME_MILLIS so that we don't forget.

@rdblue

rdblue commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

Thanks, Anton!

@ashleycoxley

Copy link
Copy Markdown
Contributor Author

Thanks both! I'll plan to pick that up soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TIME_MICROS not supported for reading (Parquet)

3 participants