You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "aiguofer (via GitHub)" <gi...@apache.org> on 2023/11/15 16:23:40 UTC
[PR] GH-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
aiguofer opened a new pull request, #38733:
URL: https://github.com/apache/arrow/pull/38733
This PR adds support for binding Arrays to Prepared Statements in the Arrow Flight SQL JDBC driver. This has only been tested locally by tweaking `ArrowFlightPreparedStatementTest` but more thorough testing of all types will be done in a follow up.
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1819826107
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit f98a13250d10dba248a2bb85989d6b80265e82d8.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/18867191198) has more details. It also includes information about 3 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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1813065924
@jduo yeah you're right.... There's a task to create better tests for parameter binding, but since this is a bit of a complex one I'll just add an extra Array parameter to the existing tests.
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1813227531
> However `FixedSizeList` doesn't have `.endValue`, I guess because it's not dynamically sized. Would I just need to verify that the number of values matches and just ignore the `.endValue` call?
Yes, that should work.
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1394619966
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/ListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,22 @@ public ListAvaticaParameterConverter(ArrowType.List type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof ListVector) {
+ ListVector listVector = ((ListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+
+ int startPos = listVector.startNewValue(index);
+ for (int i = 0; i < values.size(); i++) {
+ childVector.getField().getType().accept(
+ new AvaticaParameterBinder.BinderVisitor(
+ childVector, TypedValue.ofSerial(typedValue.componentType, values.get(i)), startPos + i));
Review Comment:
Does this have the correct behavior if an element in the input list is null?
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1395772430
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/FixedSizeListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,33 @@ public FixedSizeListAvaticaParameterConverter(ArrowType.FixedSizeList type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof FixedSizeListVector) {
+ FixedSizeListVector listVector = ((FixedSizeListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+ if (values.size() > listVector.getListSize()) {
Review Comment:
Should it be `values.size() != listVector.getListSize()`?
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1395976591
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/FixedSizeListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,33 @@ public FixedSizeListAvaticaParameterConverter(ArrowType.FixedSizeList type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof FixedSizeListVector) {
+ FixedSizeListVector listVector = ((FixedSizeListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+ if (values.size() > listVector.getListSize()) {
Review Comment:
For any vector, if you set value count to more items than you've actually set, the rest just get set to `null` right? when we set the listvector's value count it handles setting the value count on the child vector.
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1813222797
Hmmm I forgot about `FixedSizeList` and `LargeList`. It looks like I can follow the same pattern for `LargeList` but using `long` instead of `int`. However `FixedSizeList` doesn't have `.endValue`, I guess because it's not dynamically sized. Would I just need to verify that the number of values matches and just ignore the `.endValue` call?
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1395938808
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/FixedSizeListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,33 @@ public FixedSizeListAvaticaParameterConverter(ArrowType.FixedSizeList type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof FixedSizeListVector) {
+ FixedSizeListVector listVector = ((FixedSizeListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+ if (values.size() > listVector.getListSize()) {
Review Comment:
I guess it depends on intended behavior. I was thinking you can have less items and they'll just get padded with `null` if missing. I jus realized I should also check if nullable for that case.
How are `FixedSizeListVectors` generally used? I can change it to a strict check or add the nullability check to see if it can hold less items.
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #38733:
URL: https://github.com/apache/arrow/pull/38733
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1395955882
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/FixedSizeListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,33 @@ public FixedSizeListAvaticaParameterConverter(ArrowType.FixedSizeList type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof FixedSizeListVector) {
+ FixedSizeListVector listVector = ((FixedSizeListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+ if (values.size() > listVector.getListSize()) {
Review Comment:
We can pad, but I'm not sure if the vector does that for you or if you have to explicitly pad...
I'm not sure they'd be used much/at all in this context
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1812855320
:warning: GitHub issue #38732 **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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on code in PR #38733:
URL: https://github.com/apache/arrow/pull/38733#discussion_r1394629438
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/ListAvaticaParameterConverter.java:
##########
@@ -33,6 +37,22 @@ public ListAvaticaParameterConverter(ArrowType.List type) {
@Override
public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+ final List<?> values = (List<?>) typedValue.value;
+
+ if (vector instanceof ListVector) {
+ ListVector listVector = ((ListVector) vector);
+ FieldVector childVector = listVector.getDataVector();
+
+ int startPos = listVector.startNewValue(index);
+ for (int i = 0; i < values.size(); i++) {
+ childVector.getField().getType().accept(
+ new AvaticaParameterBinder.BinderVisitor(
+ childVector, TypedValue.ofSerial(typedValue.componentType, values.get(i)), startPos + i));
Review Comment:
Ahh good call! I forgot I pulled out the null value handling to the top `AvaticaParameterBinder.bind` method. I'll add handling for the `null` values!
--
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-38732: [Java][FlightRPC] Add support for Array parameter binding in JDBC [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38733:
URL: https://github.com/apache/arrow/pull/38733#issuecomment-1813044996
The implementation looks good but it would be nice to have some tests.
--
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