Skip to content
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

SNOW-895829: Handle Java8 date/time objects #1494

Open
fleiber opened this issue Aug 18, 2023 · 8 comments
Open

SNOW-895829: Handle Java8 date/time objects #1494

fleiber opened this issue Aug 18, 2023 · 8 comments
Assignees
Labels
feature status-triage_done Initial triage done, will be further handled by the driver team

Comments

@fleiber
Copy link

fleiber commented Aug 18, 2023

What is the current behavior?

The JDBC specification specifies a list of functions to implement, including getDate and getTimestamp, returning the old (and crappy ) java.sql.Date and java.sql.Timestamp objects. Users then have to convert them to proper Java8 date/time instances (which is inelegant and inefficient).

What is the desired behavior?

JDBC 4.1 in Java 1.7 introduced a getObject function, which was created to avoid having to add new functions for all future types, and is supposed to handle "common" data types. Since Java 8, those types should include LocalDate, Instant, LocalDateTime and ZonedDateTime.
Instead of writing rs.getTimestamp(1, tz).toInstant()
we would write: rs.getObject(1, Instant.class).
As of version 3.14.0, this function simply throws a SnowflakeLoggedFeatureNotSupportedException.

How would this improve snowflake-jdbc?

This improves the compatibility of the snowflake driver with the JDBC 4.2 specification, and avoid users having to write different code for different JDBC drivers.
The resulting user code may not seem much different, but is noticeably more efficient since it avoids the creation of a couple objects for every call.
It also largely improves the timezone mess: java.sql.Timestamp instances always contain a tz (which can be specified in the get call), which is most of the time confusing, using the proper object for your usage will avoid many developer headaches.

References, Other Background

For instance, SQL Server implements all the Java8 date/time objects : https://github.com/microsoft/mssql-jdbc/blob/master/src/main/java/com/microsoft/sqlserver/jdbc/DDC.java#L1202

@github-actions github-actions bot changed the title Handle Java8 date/time objects SNOW-895829: Handle Java8 date/time objects Aug 18, 2023
@sfc-gh-spanaite
Copy link
Contributor

FYI @sfc-gh-anugupta

@rickcr
Copy link

rickcr commented Apr 8, 2024

I just want to confirm this is an issue for us (or probably anyone using these Java types in their domain. Using MyBatis, I had to create a TypeHandler in order to handle this LocalDate conversion (which any other modern drivers do not have this issue.) Since LocalDate/ Time etc have been around since Java 8 (2014), I'd consider this a high priority to be addressed in your JDBC driver. Thanks, Rick Reumann

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Apr 26, 2024
@rlanhellas
Copy link

Any news here ? I am trying to receive a LocalDateTime on setObject() but got Cast exception since it just accepts java.sql.* types.

@sfc-gh-dszmolka
Copy link
Contributor

Due to other critical priorities, the work on this particular item has been deferred to early 2025. I understand that's not the answer you were looking for.
If the lack of capability which never existed in the driver, blocks a mission-critical project for you (even with the workaround of converting the objects which is already available), please reach out to your Snowflake account team and tell them how important this enhancement would be for you. They can work with the Product team directly.

Alternatively if anyone has the time and resources to implement this capability, a PR is more than welcome.

Thank you folks for bearing with us while this gets implemented, and sorry to bring such news.

@rlanhellas
Copy link

In my specific case I have troubles with setObject() not getObject() but I can try to work on this PR

@basclaessen
Copy link

I also developed a custom type handler for Hibernate to address this issue. The sample code provided below can also be utilized in other scenarios:

    @Override
    public ZonedDateTime nullSafeGet(ResultSet rs, int position, SharedSessionContractImplementor session, Object owner) throws SQLException {
        final SnowflakeTimestampWithTimezone snowflakeTimestampWithTimezone = rs.getObject(position, SnowflakeTimestampWithTimezone.class);
        if (snowflakeTimestampWithTimezone == null) {
            return null;
        }
        return snowflakeTimestampWithTimezone.toZonedDateTime();
    }

@rlanhellas
Copy link

I do have a draft PR, at least to fix setObject() case , tested and it's working. But I don't have permissions to create branches. What's the process to open up a PR over here?

@sfc-gh-dszmolka
Copy link
Contributor

looking over the PRs submitted here, many of them are from external contributors (here, here, etc)) so if you're really willing to take a shot at fixing it it's highly appreciated!

Please note the tests probably won't start running for your PR, you'll need someone from Snowflake to do it for you, but besides that I think it's fine you contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

8 participants