You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2023/01/18 18:31:58 UTC
[GitHub] [calcite] tjbanghart opened a new pull request, #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
tjbanghart opened a new pull request, #3034:
URL: https://github.com/apache/calcite/pull/3034
Lays the groundwork for [CALCITE-2980](https://issues.apache.org/jira/browse/CALCITE-2980)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080677876
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementAlias.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Represents a format element comprised of one or more {@link FormatElementEnum} entries.
+ */
+public class FormatModelElementAlias implements FormatModelElement {
Review Comment:
Yeah exactly - it maps to one or more FormatModelElementEnum entries.
For example in Google SQL `%R` is the same as `HH24:MI`. I'll add more context/examples in the javadoc
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1092488598
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
Review Comment:
I removed this operator class and moved most of the classes into the `FormatModels` utility as you had suggested elsewhere. It makes sense for them to be in a single place and we no longer try to string a string argument as something other than a string (i.e. no more premature optimization to convert a string into a `FormatModel`)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1092484458
##########
core/src/main/java/org/apache/calcite/sql/SqlDialect.java:
##########
@@ -1002,6 +1004,18 @@ protected static void unparseOffset(SqlWriter writer, @Nullable SqlNode offset)
}
}
+ /**
Review Comment:
In addressing this comment I tried to do both things -- formatting/parse functions are given a static `FormatModels` instance for parsing strings. This `FormatModels` receives its element mapping from the `BigQuerySqlDialect` class.
We could reverse the hierarchy if needed so that the format function holds the element mapping and the SqlDialect refers to that for unparsing.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1387591383
@tanclary yes I will add those. Thanks for reminding me!
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080608342
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -240,6 +281,17 @@ public MysqlSqlDialect(Context context) {
unparseListAggCall(writer, call, null, leftPrec, rightPrec);
break;
+ case FORMAT_DATE:
+ case FORMAT_TIME:
+ case FORMAT_TIMESTAMP:
+ case FORMAT_DATETIME:
+ writer.print("DATE_FORMAT(");
Review Comment:
Yeah that's open for discussion -- how to best swap a `FORMAT_TIMESTAMP` Google SQL style function call to any other dialect. Snowflake for example uses `TO_CHAR` or `TO_VARCHAR`.
Maybe a generic entry in `SqlKind` called `GOOGLE_SQL_DATETIME_FORMAT`?
Could even be more generic like `DATETIME_FORMAT` and then each dialect would know how to unparse.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1092488598
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
Review Comment:
I removed this operator class. Instead, I moved most of the classes into the `FormatModels` utility class as you had suggested elsewhere. It makes sense for them to be in a single place and we no longer try to string a string argument as something other than a string (i.e. no more premature optimization to convert a string into a `FormatModel`)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tanclary commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tanclary commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1073952163
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -184,6 +185,46 @@ public MysqlSqlDialect(Context context) {
return super.getCastSpec(type);
}
+ /** {@inheritDoc}
+ *
+ * <p>MySQL format element reference:
+ * <a href="https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format">
+ * MySQL Date and Time Functions</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
Makes sense! Looks great.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1430300409
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [85.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1411174425
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [87.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] olivrlee commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
olivrlee commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080644292
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementAlias.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Represents a format element comprised of one or more {@link FormatElementEnum} entries.
+ */
+public class FormatModelElementAlias implements FormatModelElement {
Review Comment:
what does this `FormatModelElementAlias` actually represent? a collection of `FormatModelElement`s?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1411024675
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [85.8% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1411084006
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [87.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1084692468
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
Review Comment:
I try not to put a hyperlink in the first sentence of javadoc.
The first sentence is special, in that it becomes the public description of the class. It should be simple, like a headline. (In fact, the 'inverted pyramid' model of journalism applies to javadoc.) Go into details (including hyperlinks) in subsequent paragraphs.
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
+
+ private final List<FormatModelElement> elements;
+ private final ImmutableMap<String, FormatModelElement> fmtModelParseMap;
Review Comment:
Field type should be `ImmutableMap<String, FormatModelElement>` rather than `Map<String, FormatModelElement>`. (I absolutely agree that the field value should be an `ImmutableMap`.) It's bad form to include implementation details in the field declaration. So, `TreeSet` bad, `SortedSet` good.
This is especially important for public fields and methods. People want to shade guava.
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -780,6 +784,29 @@ public RexNode convertWindowFunction(
return cx.getRexBuilder().makeCall(returnType, fun, exprs);
}
+ public RexNode convertFormatDatetimeFunction(
+ SqlRexContext cx,
+ SqlBigQueryFormatDatetimeFunction fun,
+ SqlCall call) {
+ final RexBuilder rexBuilder = cx.getRexBuilder();
+ final List<RexNode> exprs =
+ IntStream.range(0, call.getOperandList().size())
+ .mapToObj(i -> {
+ SqlNode op = call.operand(i);
+ // convert the first char literal arg to a format model
+ if (i == 0 && op.getClass() == SqlCharStringLiteral.class) {
Review Comment:
I think that creating a `FormatModel` as a new function at compile time is literally premature optimization. It will be difficult to write rules that work on this operator, to serialize the plan as JSON, to convert the plan back to SQL in another dialect, and many other things. Just keep the argument as a string expression.
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
+
+ private final List<FormatModelElement> elements;
+ private final ImmutableMap<String, FormatModelElement> fmtModelParseMap;
+
+ /**
+ * TODO(CALCITE-2980): This should live elsewhere and be associated with {@link SqlLibrary}
+ * or {@link org.apache.calcite.config.Lex}.
+ */
+ public static final ImmutableMap<String, FormatModelElement> BIG_QUERY_FORMAT_ELEMENT_PARSE_MAP =
+ FormatModelElement.listToMap(
+ new ImmutableList.Builder<FormatModelElement>()
+ .add(FormatModelElementAlias.create("%A", DAY))
+ .add(FormatModelElementAlias.create("%a", DY))
+ .add(FormatModelElementAlias.create("%B", MONTH))
+ .add(FormatModelElementAlias.create("%b", MON))
+ .add(FormatModelElementAlias.create("%d", DD))
+ .add(FormatModelElementAlias.create("%H", HH24))
+ .add(FormatModelElementAlias.create("%h", MON))
+ .add(FormatModelElementAlias.create("%j", DDD))
+ .add(FormatModelElementAlias.create("%k", HH24))
+ .add(FormatModelElementAlias.create("%M", MI))
+ .add(FormatModelElementAlias.create("%m", MM))
+ .add(FormatModelElementAlias.create("%Q", Q))
+ .add(
+ FormatModelElementAlias.create("%R",
+ Arrays.asList(HH24, new FormatModelElementLiteral(":"), MI),
+ "The time in the format %H:%M"))
+ .add(FormatModelElementAlias.create("%S", SS))
+ .add(FormatModelElementAlias.create("%U", WW))
+ .add(FormatModelElementAlias.create("%u", D))
+ .add(FormatModelElementAlias.create("%V", IW))
+ .add(FormatModelElementAlias.create("%W", WW))
+ .add(FormatModelElementAlias.create("%Y", YYYY))
+ .add(FormatModelElementAlias.create("%Z", TZR))
+ .build());
+
+
+ public FormatModel(String fmtString, SqlLibrary library) {
Review Comment:
I think the `library` argument should go. The main association is between FormatModel and function (a subclass of SqlFunction such as SqlLibraryOperators.FORMAT_TIMESTAMP), and no direct association with either `SqlLibrary` or `SqlDialect`.
Later, there will be an association between `SqlDialect` and `SqlFunction`, namely, 'in the MySQL dialect, which is the equivalent of the `FORMAT_TIMESTAMP` function' but that can come later.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementAlias.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Represents a format element comprised of one or more {@link FormatElementEnum} entries.
+ */
+public class FormatModelElementAlias implements FormatModelElement {
Review Comment:
As I mentioned previously, I think this should be renamed 'CompositeFormatElement'.
Consider moving into a helper class, `FormatModels`, and access via a method `public static FormatModelElement compositeElement(String, List<FormatModelElement>, String)`. It's possible that the inner class can be private.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementLiteral.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import java.util.Objects;
+
+/**
+ * Represents literal text in a format string.
+ */
+public class FormatModelElementLiteral implements FormatModelElement {
+
Review Comment:
Consider moving into a helper class, `FormatModels.literalElement(String)`.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ */
+ public static List<FormatModelElement> parse(String format,
+ ImmutableMap<String, FormatModelElement> fmtModelMap) {
+ List<FormatModelElement> elements = new ArrayList<>();
+ // TODO(CALCITE-2980): make these regex patterns static and tied to a library or dialect.
+ StringBuilder regex = new StringBuilder();
+ for (String key : fmtModelMap.keySet()) {
+ regex.append("(").append(Pattern.quote(key)).append(")|");
+ }
+ // remove the last '|'
+ regex.setLength(regex.length() - 1);
+ Matcher matcher = Pattern.compile(regex.toString()).matcher(format);
Review Comment:
Compiling a regex each call is expensive. That implies that after you have created `fmtModelMap` you then build a "parser" object that compiles the regex and stores the `Pattern`.
##########
core/src/test/java/org/apache/calcite/util/FormatModelUtilTest.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.sql.FormatModel;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableMap;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Unit test for {@link FormatModelUtil}.
+ */
+public class FormatModelUtilTest {
+
+ private static final ImmutableMap TEST_MAP = FormatModel.BIG_QUERY_FORMAT_ELEMENT_PARSE_MAP;
+
+ private void assertFormatElementParse(String fmtStr, List<String> expected) {
Review Comment:
switch the 'expected' argument to a matcher, like this:
```
private void assertThatFormatElementParse(String fmtStr, Matcher<List<String>> matcher) {
```
so that you can write tests so that they can be read left-to-right in English:
```
assertThatFormatElementParse("%j", is(Arrays.asList("DDD")));
```
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElement.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * A format element in a format string. Knows how to parse and unparse itself.
+ */
+public interface FormatModelElement {
+ /** TODO(CALCITE-2980): use these methods when implementing format functions. */
+ // String unparseFromDate(java.util.Date date);
+ // String unparseFromInt(Integer date);
+ // String unparseFromLong(Long timestamp);
+
+ /**
+ * Returns the literal value of an element.
+ *
+ * <p>For example, {@code %H} in MySQL represents the hour in 24-hour format (e.g., 00..23). This
+ * method returns the string literal "%H".
+ */
+ String getLiteral();
+
+ /**
+ * Returns the {@link FormatElementEnum} token representing the element.
+ *
+ * <p>For example, {@code %H} in MySQL represents the hour in 24-hour format (e.g., 00..23). This
+ * method returns the string "HH24" which is the name of the FormatElementEnum {@code HH24}
+ */
+ String getToken();
+
+ /**
+ * Returns the description of an element.
+ *
+ * <p>For example, {@code %H} in MySQL represents the hour in 24-hour format (e.g., 00..23). This
+ * method returns the string "The hour (24-hour clock) as a decimal number (00-23)" which is the
+ * description of the FormatElementEnum {@code HH24}
+ */
+ String getDescription();
+
+ /**
+ * Whether this element is a {@link FormatModelElementAlias}. An alias comprises one or
+ * more standard elements declared in {@link FormatElementEnum}.
+ *
+ * <p>For example, {@code %R} in Google SQL represents the hour in 24-hour format (e.g., 00..23)
+ * followed by the minute as a decimal number. This method would return {@code true} as "%R" is
+ * an alias for the standard elements {@code HH24} and {@code MI}.
+ */
+ default boolean isAlias() {
Review Comment:
`isAlias` makes you think that there are only two subtypes but there are 3 or 4 - built in, literal, composite, and maybe other user-defined.
I would replace both `isAlias` and `getElements` with a method
```
default void flatten(Consumer<FormatModelElement> consumer) {
consumer.accept(this);
}
```
that is overridden in the composite format element.
It's possible that format elements shouldn't have names. The names are just keys by which they are included in a map. So the 'alias' behavior is better described as 'composite'.
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -271,6 +272,50 @@ private static TimeUnit validate(TimeUnit timeUnit) {
}
}
+ /** {@inheritDoc}
+ *
+ * <p>BigQuery format element reference:
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/format-elements">
+ * BigQuery Standard SQL Format Elements</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
I agree with @wnob that this `switch` should go, and it should use a map. Probably the map is bi-directional. Probably the map is the parse map, and that parse map can evolve from an ImmutableMap to a larger class.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElement.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * A format element in a format string. Knows how to parse and unparse itself.
+ */
+public interface FormatModelElement {
Review Comment:
I would rename 'FormatModelElement' to 'FormatElement'. It's shorter, and several subclasses follow this convention already.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
Review Comment:
following modern naming conventions, I would rename `FormatModelUtil` to `FormatModels`.
I would move various subclasses of `FormatModelElement` into this class as inner classes. With luck - if we have designed the API correctly - these inner classes can be private.
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
Review Comment:
`FormatModel` should not extend `SqlInternalOperator`. Operators (and functions) should be fixed i.e. stored in public static final fields.
There should be a way to convert a string to a `FormatModel`. That should be a property of each function (e.g. FORMAT_TIMESTAMP has one, and CAST has another. It is probably invoked at run time, i.e. the first time the function is called.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementAlias.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Represents a format element comprised of one or more {@link FormatElementEnum} entries.
+ */
+public class FormatModelElementAlias implements FormatModelElement {
+
+ private String alias;
+ private String description;
+ private List<FormatModelElement> formatElements;
+
+ private FormatModelElementAlias(String alias, List<FormatModelElement> formatElements,
+ String description) {
+ this.alias = Objects.requireNonNull(alias, "element alias");
+ this.formatElements = Objects.requireNonNull(formatElements, "format elements");
Review Comment:
make a copy, using `ImmutableList.copyOf`
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1859,6 +1859,49 @@ private SqlDialect nonOrdinalDialect() {
sql(timeTrunc).withLibrary(SqlLibrary.BIG_QUERY).ok(expectedTimeTrunc);
}
+ @Test void testBigQueryDatetimeFormatFunctions() {
+ final String formatTime = "select format_time('%H', time '12:45:30')\n"
+ + "from \"foodmart\".\"product\"\n";
+ final String formatDate = "select format_date('%b-%d-%Y', date '2012-02-03')\n"
+ + "from \"foodmart\".\"product\"\n";
+ final String formatTimestamp =
+ "select format_timestamp('%b-%d-%Y', timestamp '2012-02-03 12:30:40')\n"
+ + "from \"foodmart\".\"product\"\n";
+ final String formatDatetime = "select format_datetime('%R', timestamp '2012-02-03 12:34:34')\n"
+ + "from \"foodmart\".\"product\"\n";
+
+ final String expectedBqFormatTime =
+ "SELECT FORMAT_TIME('%H', TIME '12:45:30')\nFROM foodmart.product";
Review Comment:
I would break each of these strings after `\n`, so
```
final String expectedBqFormatTime =
"SELECT FORMAT_TIME('%H', TIME '12:45:30')\nFROM foodmart.product";
```
becomes
```
final String expectedBqFormatTime =
"SELECT FORMAT_TIME('%H', TIME '12:45:30')\n"
+ "FROM foodmart.product";
```
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementLiteral.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import java.util.Objects;
+
+/**
+ * Represents literal text in a format string.
+ */
+public class FormatModelElementLiteral implements FormatModelElement {
+
+ private String literal;
Review Comment:
`final`
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
+
+ private List<FormatModelElement> elements;
+ private ImmutableMap<String, FormatModelElement> fmtModelParseMap;
+
+ /**
+ * TODO(CALCITE-2980): This should live elsewhere and be associated with {@link SqlLibrary}
+ * or {@link org.apache.calcite.config.Lex}.
+ */
+ public static final ImmutableMap<String, FormatModelElement> BIG_QUERY_FORMAT_ELEMENT_PARSE_MAP =
Review Comment:
As discussed in the review, the world knows only BigQuery SQL so it's simpler to call it BigQuery SQL. I agree that Google SQL is more correct but it's more important to be understood than to be 100% correct.
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElementAlias.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Represents a format element comprised of one or more {@link FormatElementEnum} entries.
+ */
+public class FormatModelElementAlias implements FormatModelElement {
+
+ private String alias;
Review Comment:
all fields should be `final`; once they are `final`, they might as well be package-private
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -184,6 +185,46 @@ public MysqlSqlDialect(Context context) {
return super.getCastSpec(type);
}
+ /** {@inheritDoc}
+ *
+ * <p>MySQL format element reference:
+ * <a href="https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format">
+ * MySQL Date and Time Functions</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
As I said previously, the association should be between the FORMAT function (as specified by MySQL) and the format element, not intermediated by the dialect object.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1430426202
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [85.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1073950252
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -184,6 +185,46 @@ public MysqlSqlDialect(Context context) {
return super.getCastSpec(type);
}
+ /** {@inheritDoc}
+ *
+ * <p>MySQL format element reference:
+ * <a href="https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format">
+ * MySQL Date and Time Functions</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
Each dialect might have a different set of elements to represent the same type of datetime unit.
For example an abbreviated month name like `Jan` is `%b` in MySQL and `MON` in Snowflake. Each `SqlDialect` will have to override this method to supply the correct string for the element type. I only did MySQL, BigQuery, and Snowflake in the first commit because I wanted to get some feedback before working on each dialect.
Oracle's format elements were chosen as the "base" set since they map closely to the SQL:2016 standard as described in [CALCITE-2980](https://issues.apache.org/jira/browse/CALCITE-2980).
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1399640965
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [71.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [5.4% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1092479733
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -271,6 +272,50 @@ private static TimeUnit validate(TimeUnit timeUnit) {
}
}
+ /** {@inheritDoc}
+ *
+ * <p>BigQuery format element reference:
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/format-elements">
+ * BigQuery Standard SQL Format Elements</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
As a followup to this PR, we can define non-standard element maps per dialect. Then, when we unparse a formatting function we would use the map to find the appropriate element string for the "standard" element. The map would be bi-directional as suggested so that it can be used as both the parse map for functions and for converting elements from one dialect to another.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tanclary commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tanclary commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1073936097
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -184,6 +185,46 @@ public MysqlSqlDialect(Context context) {
return super.getCastSpec(type);
}
+ /** {@inheritDoc}
+ *
+ * <p>MySQL format element reference:
+ * <a href="https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format">
+ * MySQL Date and Time Functions</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
Why does this logic go in MySqlSqlDialect?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mkou commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "mkou (via GitHub)" <gi...@apache.org>.
mkou commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1084652713
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -30,44 +32,34 @@ private FormatModelUtil() {}
/**
* Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
- *
- * TODO(CALCITE-2980): make this configurable for multiple parse maps. Currently this only works
- * for BigQuery and MySQL style format strings where elements begin with '%'
*/
- public static List<FormatModelElement> parse(String fmtString,
- ImmutableMap<String, FormatModelElement> fmtModel) {
- char ch;
- int idx = 0;
- StringBuilder buf = new StringBuilder();
- Stack elements = new Stack();
- while (idx < fmtString.length()) {
- switch (ch = fmtString.charAt(idx)) {
- case '%':
- String key = buf.append(fmtString, idx, idx + 2).toString();
- FormatModelElement element = fmtModel.get(key);
- if (element == null) {
- elements.push(new FormatModelElementLiteral(buf.toString()));
- } else {
- elements.push(element);
- }
- buf.setLength(0);
- idx += 2;
- break;
- default:
- buf.append(ch);
- // consume literal elements up to end of string or next '%'
- while (idx < fmtString.length() - 1) {
- ch = fmtString.charAt(++idx);
- if (ch == '%') {
- --idx;
- break;
- }
- buf.append(ch);
- }
- elements.push(new FormatModelElementLiteral(buf.toString()));
- buf.setLength(0);
- ++idx;
+ public static List<FormatModelElement> parse(String format,
Review Comment:
One thing that I realize is missing is the separator that is available in some dialects like redshift :/
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1412538588
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [87.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asfgit closed pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
URL: https://github.com/apache/calcite/pull/3034
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1081843370
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlBigQueryFormatDatetimeFunction.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql.fun;
+
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeFamily;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import java.util.Locale;
+
+import static org.apache.calcite.sql.type.SqlTypeName.DATE;
+import static org.apache.calcite.sql.type.SqlTypeName.TIME;
+
+/**
+ * <p> The Google BigQuery style datetime formatting functions. This is a generic type representing
+ * one of the following:</p>
+ *
+ * <ul>
+ * <li>{@code FORMAT_TIME(format_string, time_object)}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/time_functions#format_time">ref</a></li>
+ * <li>{@code FORMAT_DATE(format_string, date_expr)}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions#format_date">ref</a></li>
+ * <li>{@code FORMAT_TIMESTAMP(format_string, timestamp[, time_zone])}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/timestamp_functions#format_timestamp">ref</a></li>
+ * <li>{@code FORMAT_DATETIME(format_string, timestamp[, time_zone])}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/datetime_functions#format_datetime">ref</a></li>
+ * </ul>
+ */
+public class SqlBigQueryFormatDatetimeFunction extends SqlFunction {
Review Comment:
Parsing/formatting functions might have enough commonality to warrant a common class - an extension to `SqlBasicFunction` with an extra field, a map from strings to format elements.
I wouldn't change it now. But come back and refactor if that pattern emerges.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1081837725
##########
core/src/main/java/org/apache/calcite/sql/SqlDialect.java:
##########
@@ -1002,6 +1004,18 @@ protected static void unparseOffset(SqlWriter writer, @Nullable SqlNode offset)
}
}
+ /**
Review Comment:
There's a complex mapping between format elements and dialects. But I suspect that SqlDialect is not the right place to do it, and functions is closer to the right place.
Hypothetically, someone could be in MySQL dialect and want to enable MySQL's and Postgres's formatting functions. It's a useful hypothetical. SqlDialect is used only for a short part of the query life cycle - when going from AST nodes to SQL text. But functions are around for longer.
I have in mind a map from strings to format elements that is part of the `SqlLibraryOperators.FORMAT_TIMESTAMP` function, and the same or similar map that is used by `FORMAT_TIME` and `PARSE_TIMESTAMP` functions. Parse functions might have an additional parser, because parsing format strings is not always trivial.
Maybe the aforementioned 'map' and 'parser' would evolve into slightly larger classes.
When we tackle the different issue for 'how do I translate a MySQL format string to a BigQuery format string?' - or 'how do I translate a call to MySQL_FORMAT_TIMESTAMP to a call to BigQuery_FORMAT_TIMESTAMP' - then those larger classes could be used to drive the proceedings.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mkou commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "mkou (via GitHub)" <gi...@apache.org>.
mkou commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1084676486
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ */
+ public static List<FormatModelElement> parse(String format,
+ ImmutableMap<String, FormatModelElement> fmtModelMap) {
+ List<FormatModelElement> elements = new ArrayList<>();
+ // TODO(CALCITE-2980): make these regex patterns static and tied to a library or dialect.
+ StringBuilder regex = new StringBuilder();
+ for (String key : fmtModelMap.keySet()) {
+ regex.append("(").append(Pattern.quote(key)).append(")|");
Review Comment:
Make sure you get the longest match
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ */
+ public static List<FormatModelElement> parse(String format,
+ ImmutableMap<String, FormatModelElement> fmtModelMap) {
+ List<FormatModelElement> elements = new ArrayList<>();
+ // TODO(CALCITE-2980): make these regex patterns static and tied to a library or dialect.
+ StringBuilder regex = new StringBuilder();
+ for (String key : fmtModelMap.keySet()) {
+ regex.append("(").append(Pattern.quote(key)).append(")|");
+ }
+ // remove the last '|'
+ regex.setLength(regex.length() - 1);
+ Matcher matcher = Pattern.compile(regex.toString()).matcher(format);
+ int i = 0;
+ while (matcher.find()) {
+ // Add any leading literal text before next element match
+ String literal = format.substring(i, matcher.start());
+ if (!literal.isEmpty()) {
+ elements.add(new FormatModelElementLiteral(literal));
Review Comment:
For parsing and unparsing, litterals might be in specific separators (`"` for example)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1083554622
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.List;
+import java.util.Stack;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ *
+ * TODO(CALCITE-2980): make this configurable for multiple parse maps. Currently this only works
+ * for BigQuery and MySQL style format strings where elements begin with '%'
+ */
+ public static List<FormatModelElement> parse(String fmtString,
Review Comment:
Took a stab at this in most recent commit.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080614602
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
+
+ private List<FormatModelElement> elements;
+ private ImmutableMap<String, FormatModelElement> fmtModelParseMap;
+
+ /**
+ * TODO(CALCITE-2980): This should live elsewhere and be associated with {@link SqlLibrary}
+ * or {@link org.apache.calcite.config.Lex}.
+ */
+ public static final ImmutableMap<String, FormatModelElement> BIG_QUERY_FORMAT_ELEMENT_PARSE_MAP =
Review Comment:
For sure, I can open a Jira case to swap `GOOGLE_SQL` or `GSQL` with `BIG_QUERY` references -- would have to a non-breaking change for config settings etc.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1081846240
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlBigQueryFormatDatetimeFunction.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql.fun;
+
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeFamily;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import java.util.Locale;
+
+import static org.apache.calcite.sql.type.SqlTypeName.DATE;
+import static org.apache.calcite.sql.type.SqlTypeName.TIME;
+
+/**
+ * <p> The Google BigQuery style datetime formatting functions. This is a generic type representing
+ * one of the following:</p>
+ *
+ * <ul>
+ * <li>{@code FORMAT_TIME(format_string, time_object)}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/time_functions#format_time">ref</a></li>
+ * <li>{@code FORMAT_DATE(format_string, date_expr)}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions#format_date">ref</a></li>
+ * <li>{@code FORMAT_TIMESTAMP(format_string, timestamp[, time_zone])}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/timestamp_functions#format_timestamp">ref</a></li>
+ * <li>{@code FORMAT_DATETIME(format_string, timestamp[, time_zone])}
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/datetime_functions#format_datetime">ref</a></li>
+ * </ul>
+ */
+public class SqlBigQueryFormatDatetimeFunction extends SqlFunction {
Review Comment:
However I would make the constructor private now. A public constructor will prevent you from subclassing later. Use a `create` method instead, and add `withXxx` methods to change the value of parameters that are not frequently used. (Same pattern as `SqlBasicFunction`, and a pattern that is a little nearer to functional programming than object-oriented.)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mkou commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
mkou commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080572882
##########
core/src/main/java/org/apache/calcite/sql/FormatModel.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.sql;
+
+import static org.apache.calcite.util.format.FormatElementEnum.D;
+import static org.apache.calcite.util.format.FormatElementEnum.DAY;
+import static org.apache.calcite.util.format.FormatElementEnum.DD;
+import static org.apache.calcite.util.format.FormatElementEnum.DDD;
+import static org.apache.calcite.util.format.FormatElementEnum.DY;
+import static org.apache.calcite.util.format.FormatElementEnum.HH24;
+import static org.apache.calcite.util.format.FormatElementEnum.IW;
+import static org.apache.calcite.util.format.FormatElementEnum.MI;
+import static org.apache.calcite.util.format.FormatElementEnum.MM;
+import static org.apache.calcite.util.format.FormatElementEnum.MON;
+import static org.apache.calcite.util.format.FormatElementEnum.MONTH;
+import static org.apache.calcite.util.format.FormatElementEnum.Q;
+import static org.apache.calcite.util.format.FormatElementEnum.SS;
+import static org.apache.calcite.util.format.FormatElementEnum.TZR;
+import static org.apache.calcite.util.format.FormatElementEnum.WW;
+import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+
+import org.apache.calcite.sql.fun.SqlLibrary;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.util.format.FormatElementEnum;
+import org.apache.calcite.util.format.FormatModelElement;
+import org.apache.calcite.util.format.FormatModelElementAlias;
+import org.apache.calcite.util.format.FormatModelElementLiteral;
+import org.apache.calcite.util.format.FormatModelUtil;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Arrays;
+import java.util.List;
+
+
+/** A <a href="https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlqr/Format-Models.html">
+ * format model</a> is a character literal that describes the format of {@code DATETIME} or {@code
+ * NUMBER} data stored in a character string.
+ *
+ * <p>{@link #unparse(SqlWriter, SqlCall, int, int)} calls
+ * {@link SqlDialect#getFormatElement(FormatElementEnum)} for known elements and aliases. Consider
+ * overriding this method if a dialect's format elements differs from those in {@link
+ * FormatElementEnum}
+ */
+public class FormatModel extends SqlInternalOperator {
+
+ private List<FormatModelElement> elements;
+ private ImmutableMap<String, FormatModelElement> fmtModelParseMap;
+
+ /**
+ * TODO(CALCITE-2980): This should live elsewhere and be associated with {@link SqlLibrary}
+ * or {@link org.apache.calcite.config.Lex}.
+ */
+ public static final ImmutableMap<String, FormatModelElement> BIG_QUERY_FORMAT_ELEMENT_PARSE_MAP =
Review Comment:
nit: I know it's everywhere so no need to change it but in general I hake a bit when I see BigQuery since we really want to do "Google SQL"
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -271,6 +272,50 @@ private static TimeUnit validate(TimeUnit timeUnit) {
}
}
+ /** {@inheritDoc}
+ *
+ * <p>BigQuery format element reference:
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/format-elements">
+ * BigQuery Standard SQL Format Elements</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
Yes I like this 🌷
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -240,6 +281,17 @@ public MysqlSqlDialect(Context context) {
unparseListAggCall(writer, call, null, leftPrec, rightPrec);
break;
+ case FORMAT_DATE:
+ case FORMAT_TIME:
+ case FORMAT_TIMESTAMP:
+ case FORMAT_DATETIME:
+ writer.print("DATE_FORMAT(");
Review Comment:
Like all of them should alias to the same canonical form so you can read the canonical form in every language idk if that makes sense
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -240,6 +281,17 @@ public MysqlSqlDialect(Context context) {
unparseListAggCall(writer, call, null, leftPrec, rightPrec);
break;
+ case FORMAT_DATE:
+ case FORMAT_TIME:
+ case FORMAT_TIMESTAMP:
+ case FORMAT_DATETIME:
+ writer.print("DATE_FORMAT(");
Review Comment:
This feels a bit weird for me but I might miss some context.
Should this live in the definition of FORMAT_DATETIME that DATE_FORMAT is actually an aliases with the operands in different order + different formats elements?
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelElement.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * A format element in a format string. Knows how to parse and unparse itself.
+ */
+public interface FormatModelElement {
Review Comment:
Adding examples for each methods would help take out some of the mental load of figuring out exactly the differences. (Easier to read) even though the names are are self suffiscient that would help
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.List;
+import java.util.Stack;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ *
+ * TODO(CALCITE-2980): make this configurable for multiple parse maps. Currently this only works
+ * for BigQuery and MySQL style format strings where elements begin with '%'
+ */
+ public static List<FormatModelElement> parse(String fmtString,
Review Comment:
Not sure if this is a good advice or not but it looks like a good case to just use ReGex and look for the formats defined by each dialect
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tanclary commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tanclary commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1387559274
Should you add descriptions for the functions to reference.md?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by GitBox <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1080688763
##########
core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java:
##########
@@ -240,6 +281,17 @@ public MysqlSqlDialect(Context context) {
unparseListAggCall(writer, call, null, leftPrec, rightPrec);
break;
+ case FORMAT_DATE:
+ case FORMAT_TIME:
+ case FORMAT_TIMESTAMP:
+ case FORMAT_DATETIME:
+ writer.print("DATE_FORMAT(");
Review Comment:
Or better yet convert these BQ format functions into the standard `CAST(... as FORMAT <str>)` and unparse the `CAST` to `DATE_FORMAT`, `TO_CHAR` accordingly.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3034:
URL: https://github.com/apache/calcite/pull/3034#issuecomment-1411156619
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3034)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3034&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3034&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list) [87.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3034&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1092490830
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.util.format;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class used to convert format strings into {@link FormatModelElement}s.
+ */
+public class FormatModelUtil {
+
+ private FormatModelUtil() {}
+
+ /**
+ * Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
+ */
+ public static List<FormatModelElement> parse(String format,
+ ImmutableMap<String, FormatModelElement> fmtModelMap) {
+ List<FormatModelElement> elements = new ArrayList<>();
+ // TODO(CALCITE-2980): make these regex patterns static and tied to a library or dialect.
+ StringBuilder regex = new StringBuilder();
+ for (String key : fmtModelMap.keySet()) {
+ regex.append("(").append(Pattern.quote(key)).append(")|");
+ }
+ // remove the last '|'
+ regex.setLength(regex.length() - 1);
+ Matcher matcher = Pattern.compile(regex.toString()).matcher(format);
Review Comment:
Formatting functions should be able to create a `FormatModels` instance that will compile the regex from the parse map during initialization. Calls to `parse` should now use the precomputed regex.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1086039023
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -271,6 +272,50 @@ private static TimeUnit validate(TimeUnit timeUnit) {
}
}
+ /** {@inheritDoc}
+ *
+ * <p>BigQuery format element reference:
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/format-elements">
+ * BigQuery Standard SQL Format Elements</a>.
+ */
+ @Override public String getFormatElement(FormatElementEnum fmtElement) {
Review Comment:
This doesn't mean we should get rid of the `getFormatElement` method (or at least something like it) entirely right?
A `SqlDialect` would still need to know how to get an appropriate format string from a BigQuery style one while unparsing. For example, `FORMAT_TIMESTAMP(TIMESTAMP '2012-12-02 12:05:00', '%R')` would need to unparse as`TO_CHAR( TIMESTAMP '2012-12-02 12:05:00', 'HH24:MI')` for Postgres based dialects.
As you mentioned elsewhere, this translation should not happen in a `FormatModel` _operator_ but rather the formatting function itself. That is, we should reach into the `SqlDialect` for this element mapping while in `SqlBigQueryFormatDatetimeFunction#unparse`
For library specific functions this mapping could also be used as the parse map.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mkou commented on a diff in pull request #3034: [CALCITE-5357] Implement BigQuery FORMAT_TIMESTAMP, FORMAT_DATE, FORMAT_TIME, and FORMAT_DATETIME
Posted by "mkou (via GitHub)" <gi...@apache.org>.
mkou commented on code in PR #3034:
URL: https://github.com/apache/calcite/pull/3034#discussion_r1084652713
##########
core/src/main/java/org/apache/calcite/util/format/FormatModelUtil.java:
##########
@@ -30,44 +32,34 @@ private FormatModelUtil() {}
/**
* Parses the {@code fmtString} using element identifiers supplied by {@code fmtModel}.
- *
- * TODO(CALCITE-2980): make this configurable for multiple parse maps. Currently this only works
- * for BigQuery and MySQL style format strings where elements begin with '%'
*/
- public static List<FormatModelElement> parse(String fmtString,
- ImmutableMap<String, FormatModelElement> fmtModel) {
- char ch;
- int idx = 0;
- StringBuilder buf = new StringBuilder();
- Stack elements = new Stack();
- while (idx < fmtString.length()) {
- switch (ch = fmtString.charAt(idx)) {
- case '%':
- String key = buf.append(fmtString, idx, idx + 2).toString();
- FormatModelElement element = fmtModel.get(key);
- if (element == null) {
- elements.push(new FormatModelElementLiteral(buf.toString()));
- } else {
- elements.push(element);
- }
- buf.setLength(0);
- idx += 2;
- break;
- default:
- buf.append(ch);
- // consume literal elements up to end of string or next '%'
- while (idx < fmtString.length() - 1) {
- ch = fmtString.charAt(++idx);
- if (ch == '%') {
- --idx;
- break;
- }
- buf.append(ch);
- }
- elements.push(new FormatModelElementLiteral(buf.toString()));
- buf.setLength(0);
- ++idx;
+ public static List<FormatModelElement> parse(String format,
Review Comment:
One thing that I realize is missing is the separator that is available in some dialects like redshift :/
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org