You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/02/09 21:53:10 UTC

[GitHub] [druid] suneet-s opened a new pull request #9337: Fix timestamp extract fn to match postgreSQL

suneet-s opened a new pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337
 
 
   Update the timestamp extract function so that it matches the PostgreSQL docs.
   Examples from the PostgreSQL docs were added as tests for DECADE, CENTURY
   and MILLENIUM extraction.
   
   There were bugs in CENTURY and MILLENIUM that were spotted because of intelliJ
   inspections - 'Integer division in floating point context'
   
   This PR has:
   - [x] been self-reviewed.
   - [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] gianm commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377945477
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
           case CENTURY:
-            return ExprEval.of(dateTime.centuryOfEra().get() + 1);
+            return ExprEval.of(Math.ceil((double) dateTime.year().get() / 100));
           case MILLENNIUM:
             // Years in the 1900s are in the second millennium. The third millennium started January 1, 2001.
             // See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.round(Math.ceil(dateTime.year().get() / 1000)));
+            return ExprEval.of(Math.round(Math.ceil((double) dateTime.year().get() / 1000)));
 
 Review comment:
   Could it be suppressed in that one location?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377837728
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
 
 Review comment:
   ^ Please verify against Postgresql

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377837604
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
 
 Review comment:
   someone collected statistics for computers back in 101 BC 😂 
   
   I'll add tests for correctness though 👍 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r376823004
 
 

 ##########
 File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
 ##########
 @@ -7730,7 +7730,7 @@ public void testFilterOnTimeExtractWithVariousTimeUnits() throws Exception
           + "AND EXTRACT(ISODOW FROM __time) = 6\n"
           + "AND EXTRACT(ISOYEAR FROM __time) = 2000\n"
           + "AND EXTRACT(DECADE FROM __time) = 200\n"
-          + "AND EXTRACT(CENTURY FROM __time) = 21\n"
+          + "AND EXTRACT(CENTURY FROM __time) = 20\n"
           + "AND EXTRACT(MILLENNIUM FROM __time) = 2\n",
 
 Review comment:
   These values come from `BaseCalciteQueryTest#TIMESERIES_CONTEXT_DEFAULT` which is `2000-01-01T00:00:00Z`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377835431
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
 
 Review comment:
   Not sure if you need to handle BC?
   Can you add tests?
   i.e. 
   -- SELECT EXTRACT(CENTURY FROM DATE '0101-12-31 BC'); 
   -- SELECT EXTRACT(CENTURY FROM DATE '0100-12-31 BC'); 
   -- SELECT EXTRACT(MILLENNIUM FROM DATE '0001-12-31 BC'); 
   -- SELECT EXTRACT(DECADE FROM DATE '0002-12-31 BC');
   -- SELECT EXTRACT(DECADE FROM DATE '0011-01-01 BC'); 
   -- SELECT EXTRACT(DECADE FROM DATE '0012-12-31 BC'); 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r376822912
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
           case CENTURY:
-            return ExprEval.of(dateTime.centuryOfEra().get() + 1);
+            return ExprEval.of(Math.ceil((double) dateTime.year().get() / 100));
           case MILLENNIUM:
             // Years in the 1900s are in the second millennium. The third millennium started January 1, 2001.
             // See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.round(Math.ceil(dateTime.year().get() / 1000)));
+            return ExprEval.of(Math.round(Math.ceil((double) dateTime.year().get() / 1000)));
 
 Review comment:
   I thought about making this an error in our inspection profile, but `CachingCostBalancerStrategy` does this, and I'm afraid to make a change in that class given the potential implications around the balancer strategy. I'm un-aware of a way to simulate what impact changing that division would have, so I decided to leave it as is.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
maytasm3 commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377833360
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
           case CENTURY:
-            return ExprEval.of(dateTime.centuryOfEra().get() + 1);
+            return ExprEval.of(Math.ceil((double) dateTime.year().get() / 100));
           case MILLENNIUM:
             // Years in the 1900s are in the second millennium. The third millennium started January 1, 2001.
             // See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.round(Math.ceil(dateTime.year().get() / 1000)));
+            return ExprEval.of(Math.round(Math.ceil((double) dateTime.year().get() / 1000)));
 
 Review comment:
   You don't need Math.round if the result is already Math.ceil since Math.ceil returns int

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] ccaominh merged pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
ccaominh merged pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377862471
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
 
 Review comment:
   I decided it might be better not to support BC.
   
   Since in ISO format, the date needs to be specified like `-0100-12-31` to signify the year `0101-12-31 BC` This would make the code complicated for a use case that is likely never to happen 🤞 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL

Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9337: Fix timestamp extract fn to match postgreSQL
URL: https://github.com/apache/druid/pull/9337#discussion_r377972341
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java
 ##########
 @@ -141,13 +141,13 @@ public ExprEval eval(final ObjectBinding bindings)
             return ExprEval.of(dateTime.year().get());
           case DECADE:
             // The year field divided by 10, See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.floor(dateTime.year().get() / 10));
+            return ExprEval.of(dateTime.year().get() / 10);
           case CENTURY:
-            return ExprEval.of(dateTime.centuryOfEra().get() + 1);
+            return ExprEval.of(Math.ceil((double) dateTime.year().get() / 100));
           case MILLENNIUM:
             // Years in the 1900s are in the second millennium. The third millennium started January 1, 2001.
             // See https://www.postgresql.org/docs/10/functions-datetime.html
-            return ExprEval.of(Math.round(Math.ceil(dateTime.year().get() / 1000)));
+            return ExprEval.of(Math.round(Math.ceil((double) dateTime.year().get() / 1000)));
 
 Review comment:
   Good suggestion - done.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org