-
Notifications
You must be signed in to change notification settings - Fork 90
GH-853: Allow ISO String Binding for Date, Time, and Timestamp #852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
GH-853: Allow ISO String Binding for Date, Time, and Timestamp #852
Conversation
Can you create an issue for this, and properly name the PR? ("GH-NNN: Allow binding ISO timestamp strings to date/time types" or something) |
Yes, I have been waiting to receive approval to create a ticket on ASF for Arrow Project. I have sent an email to the dev mail - dev@arrow.apache.org. Once I create the issue I will add more details to the PR. Is there anything more I can do to expedite this process of receiving access? |
We use GitHub issues. You should not need approval. |
(Note that we do NOT use JIRA.) |
FYI: You can create a new issue from https://github.com/apache/arrow-java/issues/new/choose . |
Fixes #852 |
Thank you for the quick responses and the links, I have linked the PR to the issue created. |
Can you fix the title? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding new tests!
TypedValue typedValue = TypedValue.create(ColumnMetaData.Rep.STRING.toString(), dateStr); | ||
boolean result = converter.bindParameter(vector, typedValue, 0); | ||
assertTrue(result); | ||
assertEquals((int) LocalDate.parse(dateStr).toEpochDay(), vector.get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this wrong? DateMilliVector is supposed to store millisecond timestamps representing dates (yes, this is a bad representation) so it should be toEpochDay() * 24 * 60 * 60 * 1000
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, I made the correction, thanks for pointing out :)
...test/java/org/apache/arrow/driver/jdbc/converter/impl/TimeAvaticaParameterConverterTest.java
Show resolved
Hide resolved
...java/org/apache/arrow/driver/jdbc/converter/impl/TimestampAvaticaParameterConverterTest.java
Show resolved
Hide resolved
...ain/java/org/apache/arrow/driver/jdbc/converter/impl/TimestampAvaticaParameterConverter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so we only allow timestamps with the Z
suffix - but we can bind to naive timestamp vectors too. And we don't allow timestamps without the suffix. (We could always relax that later, though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct. Currently, we only accept timestamps with the Z suffix, but we can easily relax this restriction later to support naive timestamps if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't allow binding timestamps with timezones to columns that don't have timezones in principle. Although what happens the Z suffix is probably unambiguous in this case I think if we ever allow other timezones then it becomes a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the last question I have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a developer I want to be able to do query parameter/JDBC Statement binding for Date, Time and Timestamp date types using an iso formatted string. This will help enable the use of 3rd party tools that use ISO string formats for bindings.
Given An Anybase JDBC connection and query that uses parameterization of a Date, Time and/or Timestamp field
When setParameter is called with a valid ISO formatted string
Then Anybase accepts the request and returns appropriate success responses
Sample client use case: