You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2023/09/29 12:00:29 UTC
[PR] Fix lead/lag to be usable without offset (druid)
kgyrtkirk opened a new pull request, #15057:
URL: https://github.com/apache/druid/pull/15057
In case the offset is not specified for a `LEAD` or `LAG` in a windowing expression; it leads to an AIOOBE
--
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
Re: [PR] Fix lead/lag to be usable without offset (druid)
Posted by "soumyava (via GitHub)" <gi...@apache.org>.
soumyava commented on code in PR #15057:
URL: https://github.com/apache/druid/pull/15057#discussion_r1342283951
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -88,9 +88,9 @@ public class Windowing
private static final ImmutableMap<String, ProcessorMaker> KNOWN_WINDOW_FNS = ImmutableMap
.<String, ProcessorMaker>builder()
.put("LAG", (agg) ->
- new WindowOffsetProcessor(agg.getColumn(0), agg.getOutputName(), -agg.getConstantInt(1)))
+ new WindowOffsetProcessor(agg.getColumn(0), agg.getOutputName(), -agg.getConstantInt(1, 1)))
Review Comment:
I think, If we are implying that LAG(dimension) without the offset always means 1, we should also document it somewhere
--
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
Re: [PR] Fix lead/lag to be usable without offset (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg merged PR #15057:
URL: https://github.com/apache/druid/pull/15057
--
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
Re: [PR] Fix lead/lag to be usable without offset (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #15057:
URL: https://github.com/apache/druid/pull/15057#discussion_r1342388044
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -88,9 +88,9 @@ public class Windowing
private static final ImmutableMap<String, ProcessorMaker> KNOWN_WINDOW_FNS = ImmutableMap
.<String, ProcessorMaker>builder()
.put("LAG", (agg) ->
- new WindowOffsetProcessor(agg.getColumn(0), agg.getOutputName(), -agg.getConstantInt(1)))
+ new WindowOffsetProcessor(agg.getColumn(0), agg.getOutputName(), -agg.getConstantInt(1, 1)))
Review Comment:
actually; I don't really see any existing documentation for LEAD/LAG; and because I don't see any either for windowing - I think that should be added separately along with LEAD/LAG
--
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
Re: [PR] Fix lead/lag to be usable without offset (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg commented on code in PR #15057:
URL: https://github.com/apache/druid/pull/15057#discussion_r1344089625
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -101,6 +102,7 @@ public CalciteWindowQueryTest(
@SuppressWarnings("unchecked")
public void windowQueryTest() throws IOException
{
+ assumeTrue("These tests are only run in sqlCompatible mode!", NullHandling.sqlCompatible());
Review Comment:
@imply-cheddar - Can you please confirm that its ok to support testing for window functions in null compatible mode only? It makes sense to me since we're already trying to move away from the default value mode, so it would be helpful that new features only support the null compatible mode.
--
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
Re: [PR] Fix lead/lag to be usable without offset (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg commented on code in PR #15057:
URL: https://github.com/apache/druid/pull/15057#discussion_r1345692499
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -101,6 +102,7 @@ public CalciteWindowQueryTest(
@SuppressWarnings("unchecked")
public void windowQueryTest() throws IOException
{
+ assumeTrue("These tests are only run in sqlCompatible mode!", NullHandling.sqlCompatible());
Review Comment:
Discussed with Zoltan and Eric - we're planning to support testing window function with default value mode as well since a lot of running deployments do use that feature. Zoltan will put up the change for testing that in a follow up, since that requires fixing another bug as well.
--
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