Skip to content

Conversation

@zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Apr 20, 2021

@github-actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@zhztheplayer zhztheplayer changed the title ARROW-11776: [Java][Dataset] Writing support for Dataset API ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI Apr 21, 2021
std::vector<std::shared_ptr<Buffer>> buffers;
for (const auto& buffer : array_data->buffers) {
buffers.push_back(buffer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this loop and the loop below could be combinded.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in c16e0cb, PR #10201

}

arrow::Status ReservationListenableMemoryPool::Allocate(int64_t size, uint8_t** out) {
Status ReservationListenableMemoryPool::Allocate(int64_t size, uint8_t** out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mixing style changes like this with new code makes reviews harder, please try to avoid large scale changes like this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion.

@@ -0,0 +1,55 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we using protos for this information?

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular this for the most part looks like it replicates data already defined in flatbuffers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Migrated to flatbuffers in PR #10201

checkParquetReadResult(schema, writeSupport.getWrittenRecords(), datum);

AutoCloseables.close(datum);
AutoCloseables.close(factory);
Copy link
Contributor

Choose a reason for hiding this comment

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

combine this line with the previous? close can take multiple parameters i believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not able to be combined directly. The first line calls code

public static void close(Iterable<? extends AutoCloseable> ac) throws Exception {
// this method can be called on a single object if it implements Iterable<AutoCloseable>
// like for example VectorContainer make sure we handle that properly
if (ac == null) {
return;
} else if (ac instanceof AutoCloseable) {
((AutoCloseable) ac).close();
return;
}
Exception topLevelException = null;
for (AutoCloseable closeable : ac) {
try {
if (closeable != null) {
closeable.close();
}
} catch (Exception e) {
if (topLevelException == null) {
topLevelException = e;
} else if (e != topLevelException) {
topLevelException.addSuppressed(e);
}
}
}
if (topLevelException != null) {
throw topLevelException;
}
}

second line calls
public static void close(AutoCloseable... autoCloseables) throws Exception {
close(Arrays.asList(autoCloseables));
}

@zhztheplayer zhztheplayer deleted the ARROW-11776 branch April 29, 2021 16:30
@zhztheplayer
Copy link
Member Author

Hi @emkornfield (and anyone who has been reviewing), sorry I made a mistake of deleting the original branch and the PR is now not able to reopen. Now I can only open a new PR: #10201. Sorry again for that.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants