You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "xxlaykxx (via GitHub)" <gi...@apache.org> on 2023/10/13 12:08:49 UTC
[PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
xxlaykxx opened a new pull request, #38261:
URL: https://github.com/apache/arrow/pull/38261
GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector.
Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object.
### What changes are included in this PR?
- `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's
- added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object.
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, some tests have been added to verify these changes.
### Are there any user-facing changes?
There should not be any breaking changes.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38261:
URL: https://github.com/apache/arrow/pull/38261#issuecomment-1774379369
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a376e3c21f60540ed8952b066f9367be99ba145f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17947083119) has more details. It also includes information about 15 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #38261:
URL: https://github.com/apache/arrow/pull/38261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "xxlaykxx (via GitHub)" <gi...@apache.org>.
xxlaykxx commented on PR #38261:
URL: https://github.com/apache/arrow/pull/38261#issuecomment-1767930379
@lidavidm could you plz re-review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38261:
URL: https://github.com/apache/arrow/pull/38261#issuecomment-1761410971
:warning: GitHub issue #38246 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "xxlaykxx (via GitHub)" <gi...@apache.org>.
xxlaykxx commented on PR #38261:
URL: https://github.com/apache/arrow/pull/38261#issuecomment-1772101665
@lidavidm still need re-review, plz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #38261:
URL: https://github.com/apache/arrow/pull/38261#discussion_r1358655387
##########
java/vector/src/main/java/org/apache/arrow/vector/types/pojo/Field.java:
##########
Review Comment:
Let's keep this class immutable. A method that makes a copy of the field with children is OK, but mutating the field isn't.
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java:
##########
@@ -633,7 +663,11 @@ public int getBufferSizeFor(int valueCount) {
@Override
public Field getField() {
- return new Field(getName(), fieldType, Collections.singletonList(getDataVector().getField()));
+ if (field.getChildren().contains(getDataVector().getField())) {
+ return field;
+ }
+ field.setChildren(Collections.singletonList(getDataVector().getField()));
+ return field;
Review Comment:
Instead of mutating the field here, we can keep the check, but just create (and cache) a new field if needed. IMO, if the garbage is that much of a concern, the creator of the vector should provide the expected type up front (this only exists because a ListVector can set its child type dynamically).
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/FixedSizeListVector.java:
##########
@@ -92,20 +91,43 @@ public FixedSizeListVector(String name,
CallBack unusedSchemaChangeCallback) {
super(allocator);
- this.name = name;
this.validityBuffer = allocator.getEmpty();
this.vector = ZeroVector.INSTANCE;
- this.fieldType = fieldType;
this.listSize = ((ArrowType.FixedSizeList) fieldType.getType()).getListSize();
Preconditions.checkArgument(listSize >= 0, "list size must be non-negative");
this.valueCount = 0;
this.validityAllocationSizeInBytes = getValidityBufferSizeFromCount(INITIAL_VALUE_ALLOCATION);
+ this.field = new Field(name, fieldType, null);
Review Comment:
delegate this ctor to the one below?
##########
java/vector/src/test/java/org/apache/arrow/vector/TestVectorSchemaRoot.java:
##########
Review Comment:
Why was this test changed?
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java:
##########
@@ -95,7 +94,23 @@ public static ListVector empty(String name, BufferAllocator allocator) {
public ListVector(String name, BufferAllocator allocator, FieldType fieldType, CallBack callBack) {
super(name, allocator, callBack);
this.validityBuffer = allocator.getEmpty();
- this.fieldType = checkNotNull(fieldType);
+ this.field = new Field(name, fieldType, null);
Review Comment:
delegate this ctor to the one below?
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/NonNullableStructVector.java:
##########
@@ -416,7 +471,8 @@ public Field getField() {
for (ValueVector child : getChildren()) {
children.add(child.getField());
}
- return new Field(name, fieldType, children);
+ field.setChildren(children);
+ return field;
Review Comment:
caching the field is OK, but let's not mutate it.
##########
java/vector/src/main/java/org/apache/arrow/vector/ValueVector.java:
##########
@@ -134,8 +134,12 @@ public interface ValueVector extends Closeable, Iterable<ValueVector> {
TransferPair getTransferPair(String ref, BufferAllocator allocator);
+ TransferPair getTransferPair(Field field, BufferAllocator allocator);
Review Comment:
Hmm, it's unfortunate that this makes existing callsites ambiguous...we should've renamed it in the original PR.
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/LargeListVector.java:
##########
@@ -121,9 +119,27 @@ public static LargeListVector empty(String name, BufferAllocator allocator) {
*/
public LargeListVector(String name, BufferAllocator allocator, FieldType fieldType, CallBack callBack) {
super(allocator);
- this.name = name;
+ this.field = new Field(name, fieldType, null);
Review Comment:
can't this constructor delegate to the one below?
##########
java/vector/src/main/java/org/apache/arrow/vector/ValueVector.java:
##########
@@ -134,8 +134,12 @@ public interface ValueVector extends Closeable, Iterable<ValueVector> {
TransferPair getTransferPair(String ref, BufferAllocator allocator);
+ TransferPair getTransferPair(Field field, BufferAllocator allocator);
Review Comment:
While there's no docstring for the existing function, let's add a docstring for the new ones at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org