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

Removed Microsecond from Extract function #17247

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Conversation

shigarg1
Copy link
Contributor

@shigarg1 shigarg1 commented Oct 4, 2024

Description

This PR

  1. Removes microsecond as supported unit for EXTRACT function. Currently the function is returning epoch(in secs)/1000 which is incorrect.
  2. TIME_EXTRACT function can also supports MILLISECOND as the unit. Updated the docs.
Query - SELECT TIME_EXTRACT(CAST('2013-10-07 12:13:27.123457' AS TIMESTAMP), 'MILLISECOND')
Result - 123 // Expected

This change was added as part of ea4bad7

Bug

Query1 - SELECT EXTRACT(MILLISECOND FROM CAST('2013-10-07 12:13:27.123457' AS TIMESTAMP))
Result - 123 // Expected

Query2 - SELECT EXTRACT(MICROSECOND FROM CAST('2013-10-07 12:13:27.123456' AS TIMESTAMP))
Result - 1,381,148 // **Incorrect**. Correct value should be 123456

NOTE: Currently joda-time only supports millisecond precision. As of java8 release it a general recommendation to move from joda-time to new date and time API found in the java.time package.

@shigarg1
Copy link
Contributor Author

shigarg1 commented Oct 7, 2024

@kgyrtkirk Can you please help to review this. I came across this while exploring Druid.

I also want to know if there is a reason to still use joda-time ? It is generally recommended to migrate to java time since Java8. If you want I can look at the feasibility of migrating from joda-time to java time.

@@ -9241,7 +9241,6 @@ public void testFilterOnTimeExtractWithVariousTimeUnits()
testQuery(
"SELECT COUNT(*) FROM druid.foo4\n"
+ "WHERE EXTRACT(YEAR FROM __time) = 2000\n"
+ "AND EXTRACT(MICROSECOND FROM __time) = 946723\n"
Copy link
Member

Choose a reason for hiding this comment

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

🤦 it was clearly wrong; 695 ms and 946723 microsec...

@kgyrtkirk
Copy link
Member

we could possibly migrate to java.time but these joda time things will be present at a lot of places.... Bounds/Ranges/Filtration

@kgyrtkirk kgyrtkirk merged commit 6898a5a into apache:master Oct 11, 2024
90 checks passed
@adarshsanjeev adarshsanjeev added this to the 32.0.0 milestone Jan 16, 2025
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.

3 participants