You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2023/05/30 06:00:56 UTC
[GitHub] [druid] abhishekrb19 opened a new pull request, #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
abhishekrb19 opened a new pull request, #14352:
URL: https://github.com/apache/druid/pull/14352
This PR:
- Fixes the `EARLIEST_BY` and `LATEST_BY` signatures -- should be `EARLIEST_BY(expr, timeColumn)` instead of `EARLIEST(expr, timeColumn)`; similarly for `LATEST_BY` signature.
- For consistency, include the function name in the signature for all Druid functions.
This change is precursor to https://github.com/apache/druid/issues/14346.
This PR has:
- [x] been self-reviewed.
- [x] been tested in a test Druid cluster.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1212469180
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
@clintropolis, yes, the single vs no quote inconsistency is everywhere in this area. I'm not sure there's a "right" way to do this since there seems to be exceptions inside the Calcite layer itself (`GROUPING`, for example). For the most part when `OperandTypes.sequence` is not present, Calcite gives back a string with single quotes in them, but again there are exceptions for things defined in Calcite internally 😅 . I guess one option is to sanitize the strings by trimming the single quotes (or include them), but I don't feel too strongly about it. We could also do it in the web-console if needed. What do you think?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1212469180
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
@clintropolis, yes, the single vs double quotes inconsistency is everywhere in this area. I'm not sure there's a "right" way to do this since there seems to be exceptions inside the Calcite layer itself (`GROUPING`, for example). For the most part when `OperandTypes.sequence` is not present, it comes back with single quotes, but again there are exceptions😅 . I guess one option is to sanitize the strings by trimming the single quotes (or include them), but I don't feel too strongly about it. We could also do it in the web-console if needed. What do you think?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #14352:
URL: https://github.com/apache/druid/pull/14352
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1212470584
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
also, fwiw, I'm intending to use function signatures for the `SIGNATURES` column outlined in the [`INFORMATION_SCHEMA.ROUTINES` proposal]( https://github.com/apache/druid/issues/14346), by calling `org.apache.calcite.sql.type.SqlOperandTypeChecker#getAllowedSignatures`. So that's another use case in addition to how we surface validation exceptions.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1216928246
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
I scraped and fixed the ones where there is an inconsistent usage. Specifically, the inconsistent usages include:
- No or incorrect single quotes around function names
- No space between args
- Additional `\n`
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1212470584
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
also, fwiw, I'm intending to use this function signature by calling `org.apache.calcite.sql.type.SqlOperandTypeChecker#getAllowedSignaturesDruidOperatorTable` outlined in the [`INFORMATION_SCHEMA.ROUTINES` proposal]( https://github.com/apache/druid/issues/14346). So that's another use case in addition to how we surface validation exceptions.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1213945522
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
>For the most part when OperandTypes.sequence is not present, Calcite gives back a string with single quotes in them, but again there are exceptions for things defined in Calcite internally
if calcite built-ins use single quotes then we should probably add single quotes in our customized ones to be consistent-ish
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1211031009
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
these have single quotes surrounding them while others do not, is there a _right_ way to do this? I know its basically entirely cosmetic on what shows up in validation exceptions, but just wondering if there is any sort of standard as the javadocs don't seem to call out what the right thing to do is.
Also how do things that are not using `OperandTypes.sequence` present themselves?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #14352: Fix `EARLIEST_BY`/`LATEST_BY` signature and include function name in signature.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #14352:
URL: https://github.com/apache/druid/pull/14352#discussion_r1212470584
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java:
##########
@@ -168,12 +168,12 @@ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
InferTypes.RETURN_TYPE,
OperandTypes.or(
OperandTypes.sequence(
- "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ "'" + StringUtils.format("%s_BY", aggregatorType.name()) + "(expr, timeColumn)'\n",
Review Comment:
also, fwiw, I'm intending to use this function signature by calling `org.apache.calcite.sql.type.SqlOperandTypeChecker#getAllowedSignatures` outlined in the [`INFORMATION_SCHEMA.ROUTINES` proposal]( https://github.com/apache/druid/issues/14346). So that's another use case in addition to how we surface validation exceptions.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org