Skip to content

Conversation

@davisusanibar
Copy link
Contributor

PR to fix problems mention at: #206

@davisusanibar
Copy link
Contributor Author

Hi @lidavidm please could you help with a review.

System.out.println("Batch: " + count[0]++ + ", RowCount: " + vsr.getRowCount());
arrowRecordBatch.close();
});
try(ArrowReader reader = scanTask.execute()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try(ArrowReader reader = scanTask.execute()){
try (ArrowReader reader = scanTask.execute()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @toddfarmer, changed lots of places.

@toddfarmer
Copy link
Contributor

Should we add any comments around these code changes to highlight that the supplied example code is version (8.0.0)-specific?

@lidavidm
Copy link
Member

Maybe we should label the top level page, or perhaps include it in the page titles, or something?

Also, I wonder if we can just snapshot the 'old' cookbook on each release and label it as the '7.0.0 cookbook' or something

<maven.compiler.source>8</maven.compiler.source>
<maven.compiler.target>8</maven.compiler.target>
<arrow.version>7.0.0</arrow.version>
<arrow.version>8.0.0</arrow.version>
Copy link
Member

Choose a reason for hiding this comment

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

We need to get this added to the list of post-release tasks and/or make it one of the things we check as part of release.

final VectorUnloader unloader = new VectorUnloader(root);
try(ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()){
loader.load(arrowRecordBatch);
System.out.print(vsr.contentToTSVString());
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not just root.contentToTSVString()? Is there a need to demonstrate VectorLoader/VectorUnloader here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@toddfarmer toddfarmer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @davisusanibar !

});
try(ArrowReader reader = scanTask.execute()){
while (reader.loadNextBatch()) {
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
try (VectorSchemaRoot root = reader.getVectorSchemaRoot()) {

while (reader.loadNextBatch()) {
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
final VectorUnloader unloader = new VectorUnloader(root);
try(ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try(ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()){
try (ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()) {

});
try(ArrowReader reader = scanTask.execute()){
while (reader.loadNextBatch()) {
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
try (VectorSchemaRoot root = reader.getVectorSchemaRoot()) {

while (reader.loadNextBatch()) {
try(VectorSchemaRoot root = reader.getVectorSchemaRoot()) {
final VectorUnloader unloader = new VectorUnloader(root);
try(ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try(ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()){
try (ArrowRecordBatch arrowRecordBatch = unloader.getRecordBatch()) {

@davisusanibar
Copy link
Contributor Author

Maybe we should label the top level page, or perhaps include it in the page titles, or something?

Also, I wonder if we can just snapshot the 'old' cookbook on each release and label it as the '7.0.0 cookbook' or something

Ticket created: #208

@lidavidm
Copy link
Member

Note we can't deploy cookbook updates until we get the R cookbook fixed (#201)

@davisusanibar
Copy link
Contributor Author

What about stackoverflow question?

@lidavidm
Copy link
Member

Do you want to just answer it?

@davisusanibar
Copy link
Contributor Author

Do you want to just answer it?

Yes, just mention the PR in case they need to continue with their work.

@lidavidm lidavidm linked an issue May 13, 2022 that may be closed by this pull request
@lidavidm lidavidm merged commit 99ca84d into apache:main May 13, 2022
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.

[Java] Parquet reading example fails with Arrow v8.0

3 participants