gianm commented on code in PR #17894:
URL: https://github.com/apache/druid/pull/17894#discussion_r2052679521
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -350,12 +268,23 @@ public PlannerConfig getPlannerConfig()
public DateTime getLocalNow()
{
- return localNow;
+ final Object tsParam = queryContext.get(CTX_SQL_CURRENT_TIMESTAMP);
+ final DateTime utcNow;
+ if (tsParam != null) {
+ utcNow = new DateTime(tsParam, DateTimeZone.UTC);
+ } else {
+ utcNow = defaultUtcNow;
+ }
+ return utcNow.withZone(getTimeZone());
}
public DateTimeZone getTimeZone()
{
- return localNow.getZone();
Review Comment:
I'm concerned about two things here:
- Performance: This method is called a lot, and
`DateTimes.inferTzFromString` can take some time, since it has to parse strings.
- Maintainability: Someone in the future is going to think of optimizing one
of these methods by computing the value in the constructor, not realizing it
would break `SET` statements.
How about instead doing:
- add back all the class-level fields, but make them regular fields, not
`final`
- add a method that computes all the class-level fields from the context
- call that method in the constructor as well as in `addAllToQueryContext`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
