ARROW-12480: [Java][Dataset] FileSystemDataset: Support reading from a directory #10114
Conversation
zhztheplayer
force-pushed
the
ARROW-12480
branch
2 times, most recently
from
94d94c0 to
c805908
Compare
zhztheplayer
force-pushed
the
ARROW-12480
branch
from
c805908 to
86aab3f
Compare
zhztheplayer
force-pushed
the
ARROW-12480
branch
from
86aab3f to
d318abc
Compare
zhztheplayer
force-pushed
the
ARROW-12480
branch
2 times, most recently
from
e96e1d1 to
49dd067
Compare
|
@emkornfield - Are you able to have a review for this patch? Just a fundamental functionality to make Java dataset API able to read from files in folder. |
|
Ping @kou @liyafan82 @kszucs - If we have other active Java code committers? |
|
Sorry, not sure when I will be able to take a look (possibility of this weekend) but I'm generally behind on reviews. |
|
I'm not familiar with Java. @kiszk Can you review this? |
|
Sure, I will do a review. |
| datum.forEach(batch -> assertEquals(1, batch.getLength())); | ||
| checkParquetReadResult(schema, expectedJsonUnordered, datum); | ||
|
|
||
| AutoCloseables.close(datum); |
There was a problem hiding this comment.
Can we use try (... datum = ...) { ... } at line 153? So, we can remove this line.
There was a problem hiding this comment.
It seems that datum is of type List that is not AutoClosable.
There was a problem hiding this comment.
Maybe place this in a finally block?
| uri = "file://" + path; | ||
| writer = AvroParquetWriter.<GenericRecord>builder(new org.apache.hadoop.fs.Path(path)) | ||
| writer = AvroParquetWriter |
There was a problem hiding this comment.
nit: Do we need this format change?
There was a problem hiding this comment.
It's just a trivial change to keep code readable.
|
|
||
|
|
||
| public ParquetWriteSupport(String schemaName, File outputFolder) throws Exception { | ||
| avroSchema = readSchemaFromFile(schemaName); | ||
| path = outputFolder.getPath() + File.separator + "generated.parquet"; | ||
| path = outputFolder.getPath() + File.separator + "generated-" + random.nextLong() + ".parquet"; |
There was a problem hiding this comment.
I think that this change wants to get a unique name for a short period.
How about using System.currentTimeMillis()?
There was a problem hiding this comment.
Doesn't milli cause collisions? ParquetWriteSupport is instantiated rapidly during ut execution.
By the way any special reason to use time rather than random? The files can be dropped after finishing ut. Users should not be aware of the files.
|
Looks good to me with a few comments. I would like to get another review, for example, by @emkornfield or @liyafan82 |
java/dataset/src/test/java/org/apache/arrow/dataset/ParquetWriteSupport.java
Show resolved
Hide resolved
java/dataset/src/test/java/org/apache/arrow/dataset/file/TestFileSystemDataset.java
Outdated
Show resolved
Hide resolved
zhztheplayer
force-pushed
the
ARROW-12480
branch
2 times, most recently
from
222ce24 to
1a6353a
Compare
|
Thanks everyone, merging! |
|
Benchmark runs are scheduled for baseline = bbbe668 and contender = e12a454. e12a454 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
No description provided.