paul-rogers commented on a change in pull request #2284: URL: https://github.com/apache/drill/pull/2284#discussion_r685524682
########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/AgeFunction.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#assign className="GAge"/> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java"/> + +<#include "/@includes/license.ftl"/> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import javax.inject.Inject; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; +import org.apache.drill.exec.ops.ContextInformation; + +/* + * This class is generated using freemarker and the ${.template_name} template. + */ + +public class ${className} { + +<#list dateIntervalFunc.dates as fromUnit> +<#list dateIntervalFunc.dates as toUnit> + + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}To${toUnit} implements DrillSimpleFunc { + + @Param ${toUnit}Holder left; + @Param ${fromUnit}Holder right; + @Output IntervalHolder out; + + public void setup() { + } + + public void eval() { + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + java.time.LocalDateTime to = java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + + long months = from.until(to, java.time.temporal.ChronoUnit.MONTHS); + from = from.plusMonths(months); + long days = from.until(to, java.time.temporal.ChronoUnit.DAYS); + from = from.plusDays(days); + long millis = from.until(to, java.time.temporal.ChronoUnit.MILLIS); + + out.months = (int) months; + out.days = (int) days; + out.milliseconds = (int) millis; + } + } +</#list> + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}ToMidnight implements DrillSimpleFunc { + + @Param ${fromUnit}Holder right; + @Workspace java.time.LocalDateTime to; Review comment: Suggestion: explain the logic here. Not sure anyone not in on our conversation will understand what we're trying to do. ########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/AgeFunction.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#assign className="GAge"/> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java"/> + +<#include "/@includes/license.ftl"/> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import javax.inject.Inject; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; +import org.apache.drill.exec.ops.ContextInformation; + +/* + * This class is generated using freemarker and the ${.template_name} template. + */ + +public class ${className} { + +<#list dateIntervalFunc.dates as fromUnit> +<#list dateIntervalFunc.dates as toUnit> + + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}To${toUnit} implements DrillSimpleFunc { + + @Param ${toUnit}Holder left; + @Param ${fromUnit}Holder right; + @Output IntervalHolder out; + + public void setup() { + } + + public void eval() { + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + java.time.LocalDateTime to = java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); Review comment: Boy, this is a tricky area! I'm not sure that this does what we want. The incoming values are assumed to be in local time. We convert them, in place, to a UTC time, with time zone. We then convert that to a local time, stating that the values are UTC. If I have a time that is noon, it gets converted to noon, UTC. That is then converted to my local time, UTC-8, and will thus be 8 PM. That's not what we want. We want noon local time. To see this, just run the above code in a unit test on a machine with a time zone other than UTC. Compare the timestamp value before and after the conversion. I suspect they will be different. A quick check of the Javadoc did not reveal an obvious method to use to go from a local timestamp to a `LocalDateTime`. This stuff always requires lots of fiddling (at least for me) to find the right combination. ########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/TimestampDiffFunction.java ########## @@ -72,25 +72,11 @@ public void eval() { <#elseif unit == "Week"> out.value = (right.value - left.value) / 604800000; // 7 * 24 * 60 * 60 * 1000 <#elseif unit == "Month" || unit == "Quarter" || unit == "Year"> - long timeMilliseconds = left.value % org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis - - right.value % org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis; - - java.time.Period between = java.time.Period.between( - java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDate(), - java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDate()); - int days = between.getDays(); - if (timeMilliseconds < 0 && days > 0) { - // in the case of negative time value increases left operand days value - between = java.time.Period.between( - java.time.Instant.ofEpochMilli(left.value + org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis).atZone(java.time.ZoneOffset.UTC).toLocalDate(), - java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDate()); - } else if (timeMilliseconds > 0 && days < 0) { - // in the case of negative days value decreases it for the right operand - between = java.time.Period.between( - java.time.Instant.ofEpochMilli(left.value - org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis).atZone(java.time.ZoneOffset.UTC).toLocalDate(), - java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDate()); - } - int months = between.getMonths() + between.getYears() * org.apache.drill.exec.vector.DateUtilities.yearsToMonths; + + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + java.time.LocalDateTime to = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); Review comment: Again. ########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/AgeFunction.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#assign className="GAge"/> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java"/> + +<#include "/@includes/license.ftl"/> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import javax.inject.Inject; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; +import org.apache.drill.exec.ops.ContextInformation; + +/* + * This class is generated using freemarker and the ${.template_name} template. + */ + +public class ${className} { + +<#list dateIntervalFunc.dates as fromUnit> +<#list dateIntervalFunc.dates as toUnit> + + @FunctionTemplate(name = "age", Review comment: Would be worth adding a comment to state that this is for the two-argument AGE function, and that we are following Postgres. Might even want to include a link to the Postgres definition. (Which seems to be that `AGE` is the same as `to - from`.) ########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/AgeFunction.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#assign className="GAge"/> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java"/> + +<#include "/@includes/license.ftl"/> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import javax.inject.Inject; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; +import org.apache.drill.exec.ops.ContextInformation; + +/* + * This class is generated using freemarker and the ${.template_name} template. + */ + +public class ${className} { + +<#list dateIntervalFunc.dates as fromUnit> +<#list dateIntervalFunc.dates as toUnit> + + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}To${toUnit} implements DrillSimpleFunc { + + @Param ${toUnit}Holder left; + @Param ${fromUnit}Holder right; + @Output IntervalHolder out; + + public void setup() { + } + + public void eval() { + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + java.time.LocalDateTime to = java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + + long months = from.until(to, java.time.temporal.ChronoUnit.MONTHS); + from = from.plusMonths(months); + long days = from.until(to, java.time.temporal.ChronoUnit.DAYS); + from = from.plusDays(days); + long millis = from.until(to, java.time.temporal.ChronoUnit.MILLIS); + + out.months = (int) months; + out.days = (int) days; + out.milliseconds = (int) millis; + } + } +</#list> + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}ToMidnight implements DrillSimpleFunc { + + @Param ${fromUnit}Holder right; + @Workspace java.time.LocalDateTime to; + @Output IntervalHolder out; + @Inject ContextInformation contextInfo; + + public void setup() { + java.time.ZoneId zoneId = contextInfo.getRootFragmentTimeZone(); + java.time.ZonedDateTime zdtStart = contextInfo.getQueryStartInstant().atZone(zoneId); + to = zdtStart.truncatedTo(java.time.temporal.ChronoUnit.DAYS).toLocalDateTime(); + } + + public void eval() { + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); Review comment: Interestingly, here we're using an actual UTC-based Unix timestamp, so the conversion is correct. A comment to explain this will help future developers. ########## File path: exec/java-exec/src/main/codegen/templates/DateIntervalFunctionTemplates/AgeFunction.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#assign className="GAge"/> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java"/> + +<#include "/@includes/license.ftl"/> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import javax.inject.Inject; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; +import org.apache.drill.exec.ops.ContextInformation; + +/* + * This class is generated using freemarker and the ${.template_name} template. + */ + +public class ${className} { + +<#list dateIntervalFunc.dates as fromUnit> +<#list dateIntervalFunc.dates as toUnit> + + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}To${toUnit} implements DrillSimpleFunc { + + @Param ${toUnit}Holder left; + @Param ${fromUnit}Holder right; + @Output IntervalHolder out; + + public void setup() { + } + + public void eval() { + java.time.LocalDateTime from = java.time.Instant.ofEpochMilli(right.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + java.time.LocalDateTime to = java.time.Instant.ofEpochMilli(left.value).atZone(java.time.ZoneOffset.UTC).toLocalDateTime(); + + long months = from.until(to, java.time.temporal.ChronoUnit.MONTHS); + from = from.plusMonths(months); + long days = from.until(to, java.time.temporal.ChronoUnit.DAYS); + from = from.plusDays(days); + long millis = from.until(to, java.time.temporal.ChronoUnit.MILLIS); + + out.months = (int) months; + out.days = (int) days; + out.milliseconds = (int) millis; + } + } +</#list> + @FunctionTemplate(name = "age", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = FunctionTemplate.NullHandling.NULL_IF_NULL) + public static class Age${fromUnit}ToMidnight implements DrillSimpleFunc { + + @Param ${fromUnit}Holder right; + @Workspace java.time.LocalDateTime to; + @Output IntervalHolder out; + @Inject ContextInformation contextInfo; + + public void setup() { + java.time.ZoneId zoneId = contextInfo.getRootFragmentTimeZone(); + java.time.ZonedDateTime zdtStart = contextInfo.getQueryStartInstant().atZone(zoneId); + to = zdtStart.truncatedTo(java.time.temporal.ChronoUnit.DAYS).toLocalDateTime(); Review comment: Interestingly, here we're using an actual UTC-based Unix timestamp, so the conversion used earlier is the correct one. But, here, are we doing the right thing? We want a local date/time with the end date, as converted from a UTC timestamp. Instead, it seems we are converting from local time to UTC. Are we trying to do the age calcs in UTC? We should do them in local time so that a baby born at 11:00PM local time yesterday (the data value) will have an age of 2 hours for a query run at 1AM today. (This is where that definition comment would be handy: the common convention is that the baby is one day old at midnight, though in our terms, at midnight the baby is one hour old. Thus, in our convention, the person won't turn 21, say, until 11PM on their 21th birthday, though a bartender will happily serve them any time on that day.) A comment to explain this will help future developers. -- 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]
