You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2022/09/16 19:57:17 UTC
[GitHub] [phoenix] gjacoby126 commented on a diff in pull request #1501: PHOENIX-5422 Use Java8 DateTime APIs instead of joda-time APIs
gjacoby126 commented on code in PR #1501:
URL: https://github.com/apache/phoenix/pull/1501#discussion_r973244729
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ExtendedQueryExecIT.java:
##########
@@ -122,7 +122,7 @@ public void testDateFunctions() throws Exception {
// // expected
// }
- queryDateArg = "a_date >= TO_DATE('1970-1-2 23:59:59') and a_date <= TO_DATE('1970-1-3 0:0:1')";
+ queryDateArg = "a_date >= TO_DATE('1970-01-02 23:59:59') and a_date <= TO_DATE('1970-01-03 00:00:01')";
Review Comment:
Likewise, Joda can parse 1 digit hour / minute / second fields and Java 8 can't?
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/DayOfWeekFunction.java:
##########
@@ -70,8 +71,11 @@ public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
return true;
}
long dateTime = inputCodec.decodeLong(ptr, arg.getSortOrder());
- DateTime jodaDT = new DateTime(dateTime, GJChronology.getInstanceUTC());
- int day = jodaDT.getDayOfWeek();
+ Date dt = new Date(dateTime);
+ Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
+ calendar.setTime(dt);
+ int day = calendar.get(Calendar.DAY_OF_WEEK) - 1;
Review Comment:
Could we please have a comment here to explain the logic here?
##########
phoenix-core/src/main/java/org/apache/phoenix/util/DateUtil.java:
##########
@@ -72,13 +74,26 @@ public class DateUtil {
public static final String DEFAULT_TIMESTAMP_FORMAT = DEFAULT_MS_DATE_FORMAT;
public static final Format DEFAULT_TIMESTAMP_FORMATTER = DEFAULT_MS_DATE_FORMATTER;
- private static final DateTimeFormatter JULIAN_DATE_TIME_FORMATTER = new DateTimeFormatterBuilder()
- .append(ISODateTimeFormat.dateParser())
- .appendOptional(new DateTimeFormatterBuilder()
- .appendLiteral(' ').toParser())
- .appendOptional(new DateTimeFormatterBuilder()
- .append(ISODateTimeFormat.timeParser()).toParser())
- .toFormatter().withChronology(GJChronology.getInstanceUTC());
+ private static final DateTimeFormatter ISO_DATE_TIME_FORMATTER = new DateTimeFormatterBuilder()
Review Comment:
Shouldn't ISO_DATE_TIME_FORMATTER and ISO_LOCAL_DATE_TIME_FORMATTER use IsoChronology?
Note that using BritishCutoverChronology is _not_ the same as Joda's GJChronology, because GJChronology cuts over between Julian and Gregorian time in 1582 and BritishCutoverChronology cuts over in 1752.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/DefaultColumnValueIT.java:
##########
@@ -593,7 +593,7 @@ public void testDefaultExpression() throws Exception {
+ "c3 DECIMAL DEFAULT TO_NUMBER('" + DEFAULT_CURRENCY_SYMBOL + "123.33', '\u00A4###.##'),"
+ "c4 VARCHAR DEFAULT 'AB' || 'CD',"
+ "c5 CHAR(5) DEFAULT 'E' || 'F',"
- + "c6 INTEGER DEFAULT \"MONTH\"(TO_TIMESTAMP('2015-6-05'))"
+ + "c6 INTEGER DEFAULT \"MONTH\"(TO_TIMESTAMP('2015-06-05'))"
Review Comment:
So Joda will parse a slightly malformed date like 2015-6-05 but Java 8 won't?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ToDateFunctionIT.java:
##########
@@ -82,7 +82,7 @@ public void testToDate_Default() throws SQLException {
try {
callToDateFunction("TO_DATE('2015-01-27T16:17:57+00:00')");
callToDateFunction("TO_DATE('2015-01-27T16:17:57Z')");
- callToDateFunction("TO_DATE('2015-W05')");
+// callToDateFunction("TO_DATE('2015-W05')");
Review Comment:
If this isn't a valid test case anymore, we can just delete it.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/DateTimeIT.java:
##########
@@ -577,7 +577,7 @@ public void testYearFunctionDate() throws SQLException {
assertEquals(2006, callYearFunction("\"YEAR\"(TO_DATE('2006-12-13'))"));
- assertEquals(2015, callYearFunction("\"YEAR\"(TO_DATE('2015-W05'))"));
+ assertEquals(2015, callYearFunction("\"YEAR\"(TO_DATE('2015-W05-1'))"));
Review Comment:
I assume this is a case that Java 8 handles differently than Joda. I've never seen this date syntax -- what does it mean?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ToDateFunctionIT.java:
##########
@@ -101,7 +101,7 @@ public void testToTime_Default() throws SQLException {
try {
callToTimeFunction("TO_TIME('2015-01-27T16:17:57+00:00')");
callToTimeFunction("TO_TIME('2015-01-27T16:17:57Z')");
- callToTimeFunction("TO_TIME('2015-W05')");
+// callToTimeFunction("TO_TIME('2015-W05')");
Review Comment:
Likewise here can be deleted.
##########
phoenix-core/src/main/java/org/apache/phoenix/cache/TimezoneGetter.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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 maynot 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 applicablelaw 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.
+ */
+package org.apache.phoenix.cache;
+
+import java.nio.ByteBuffer;
+import java.time.ZoneId;
+import java.time.zone.ZoneRulesException;
+import java.util.TimeZone;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.schema.IllegalDataException;
+
+public class TimezoneGetter {
+
+ /**
+ * Returns java's TimeZone instance from cache or create new instance and cache it.
Review Comment:
The comment here mentions that we're caching the TimeZone as we did with Joda, but we're not. (Java 8 might or might not be doing that internally, but that's an implementation detail not part of the interface.)
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ToDateFunctionIT.java:
##########
@@ -120,7 +120,7 @@ public void testToTimestamp_Default() throws SQLException {
try {
callToTimestampFunction("TO_TIMESTAMP('2015-01-27T16:17:57+00:00')");
callToTimestampFunction("TO_TIMESTAMP('2015-01-27T16:17:57Z')");
- callToTimestampFunction("TO_TIMESTAMP('2015-W05')");
+// callToTimestampFunction("TO_TIMESTAMP('2015-W05')");
Review Comment:
Likewise here can be deleted.
##########
phoenix-core/src/test/java/org/apache/phoenix/util/DateUtilTest.java:
##########
@@ -183,10 +183,11 @@ public void testParseTimestamp_tooLargeNanos() {
DateUtil.parseTimestamp("1970-01-01 00:00:10.9999999999");
}
- @Test(expected=IllegalDataException.class)
- public void testParseTimestamp_missingNanos() {
- DateUtil.parseTimestamp("1970-01-01 00:00:10.");
- }
+// @Test(expected=IllegalDataException.class)
+// public void testParseTimestamp_missingNanos() {
Review Comment:
If the test is no longer valid then we can just delete it.
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/FloorMonthExpression.java:
##########
@@ -37,8 +40,9 @@ public FloorMonthExpression(List<Expression> children) {
}
@Override
- public long roundDateTime(DateTime datetime) {
- return datetime.monthOfYear().roundFloorCopy().getMillis();
+ public long roundDateTime(Date dateTime) {
+ return DateUtils.truncate(dateTime, Calendar.MONTH).getTime()
+ + Calendar.getInstance().getTimeZone().getRawOffset();
Review Comment:
How do we know that the default Calendar is in the right time zone?
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/FloorWeekExpression.java:
##########
@@ -36,8 +37,7 @@ public FloorWeekExpression(List<Expression> children) {
}
@Override
- public long roundDateTime(DateTime datetime) {
- return datetime.weekOfWeekyear().roundFloorCopy().getMillis();
+ public long roundDateTime(Date dateTime) {
+ return dateTime.getTime() - dateTime.getTime() % week - correction;
Review Comment:
This can't be done using the floor function in DateTimeUtils and Calendar.WEEK_OF_YEAR?
As a side note, I see in the docs in https://phoenix.apache.org/language/functions.html#floor that we only support DAY, HOUR, MINUTE, SECOND and MILLISECOND, so I'm surprised this was here...
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundWeekExpression.java:
##########
@@ -17,25 +17,36 @@
*/
package org.apache.phoenix.expression.function;
+import java.util.Calendar;
+import java.util.Date;
import java.util.List;
import org.apache.phoenix.expression.Expression;
-import org.joda.time.DateTime;
+
+import static java.lang.Math.abs;
/**
*
- * Rounds off the given {@link DateTime} to the nearest Monday.
+ * Rounds off the given {@link Date} to the nearest Monday.
*/
-public class RoundWeekExpression extends RoundJodaDateExpression {
+public class RoundWeekExpression extends RoundJavaDateExpression {
- public RoundWeekExpression(){}
+ public RoundWeekExpression() {}
public RoundWeekExpression(List<Expression> children) {
super(children);
}
@Override
- public long roundDateTime(DateTime dateTime) {
- return dateTime.weekOfWeekyear().roundHalfEvenCopy().getMillis();
+ public long roundDateTime(Date dateTime) {
+ long nextMonday = new CeilWeekExpression().roundDateTime(dateTime);
+ long prevMonday = new FloorWeekExpression().roundDateTime(dateTime);
Review Comment:
Likewise, we can't use DateUtils.round here?
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/CeilYearExpression.java:
##########
@@ -37,8 +39,9 @@ public CeilYearExpression(List<Expression> children) {
}
@Override
- public long roundDateTime(DateTime dateTime) {
- return dateTime.year().roundCeilingCopy().getMillis();
+ public long roundDateTime(Date dateTime) {
+ return DateUtils.ceiling(dateTime, Calendar.YEAR).getTime() +
+ Calendar.getInstance().getTimeZone().getRawOffset();
Review Comment:
How do we know that the default Calendar is in the right time zone?
##########
phoenix-core/src/main/java/org/apache/phoenix/expression/function/CeilWeekExpression.java:
##########
@@ -37,8 +40,7 @@ public CeilWeekExpression(List<Expression> children) {
}
@Override
- public long roundDateTime(DateTime dateTime) {
- return dateTime.weekOfWeekyear().roundCeilingCopy().getMillis();
+ public long roundDateTime(Date dateTime) {
Review Comment:
Nothing in DateUtils will do this?
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org