You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "Akshat-Jain (via GitHub)" <gi...@apache.org> on 2024/09/12 04:00:42 UTC

[PR] MSQ window functions: Reject MVDs during window processing (druid)

Akshat-Jain opened a new pull request, #17036:
URL: https://github.com/apache/druid/pull/17036

   ### Description
   
   This PR aims to reject MVDs when used as `partition by` columns during window function processing for MSQ engine.
   
   Prior to this change, a MSQ query like the following was failing with ClassCastException:
   ```sql
   select cityName, countryName, array_to_mv(array[1,length(cityName)]),
   row_number() over (partition by  array_to_mv(array[1,length(cityName)]) order by countryName, cityName)
   from wikipedia
   where countryName in ('Austria', 'Republic of Korea') and cityName is not null
   order by 1, 2, 3
   ```
   
   This is a follow-up on https://github.com/apache/druid/pull/17002, where a similar thing was done for sql-native engine.
   
   <hr>
   
   This PR has:
   
   - [x] been self-reviewed.
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] a release note entry in the PR description.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
   - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [ ] 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

Re: [PR] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "sreemanamala (via GitHub)" <gi...@apache.org>.
sreemanamala commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1766574327


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java:
##########
@@ -451,6 +453,14 @@ private boolean comparePartitionKeys(ResultRow row1, ResultRow row2, List<String
     int match = 0;
     for (String columnName : partitionColumnNames) {
       int i = frameReader.signature().indexOf(columnName);
+      if (ColumnType.STRING.equals(frameReader.signature().getColumnType(columnName).get()) && (row1.get(i) instanceof List || row2.get(i) instanceof List)) {
+        // special handling to reject MVDs
+        throw new UOE(

Review Comment:
   I guess we should throw DruidException - InvalidInput



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1756143992


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,23 @@ public void testReplaceWithPartitionedByDayOnWikipedia(String contextName, Map<S
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")

Review Comment:
   what's the point of parameterizing this test? do those parameter changes relevant for this test?



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java:
##########
@@ -451,6 +453,14 @@ private boolean comparePartitionKeys(ResultRow row1, ResultRow row2, List<String
     int match = 0;
     for (String columnName : partitionColumnNames) {
       int i = frameReader.signature().indexOf(columnName);
+      if (ColumnType.STRING.equals(frameReader.signature().getColumnType(columnName).get()) && (row1.get(i) instanceof List || row2.get(i) instanceof List)) {

Review Comment:
   this is an unfortunate place to add this check - and it would only care about partition columns
   I think the other check which was added in #17002 should get triggered...
   
   ...and actually it would if there wouldn't be a `compare` in this class....this conditional will go away along with this whole method soon; so doesn't matter - its ok to have it like this until we get to that point....



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1756163700


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,23 @@ public void testReplaceWithPartitionedByDayOnWikipedia(String contextName, Map<S
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")

Review Comment:
   I think we parameterize these tests just for better naming?
   
   The `data` itself consists of a single element:
   ```
   public static Collection<Object[]> data()
     {
       Object[][] data = new Object[][]{
           {DEFAULT, DEFAULT_MSQ_CONTEXT}
       };
   
       return Arrays.asList(data);
     }
   ```
   
   All the tests in this file are the same, so just keeping it consistent with them.



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1756160696


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java:
##########
@@ -451,6 +453,14 @@ private boolean comparePartitionKeys(ResultRow row1, ResultRow row2, List<String
     int match = 0;
     for (String columnName : partitionColumnNames) {
       int i = frameReader.signature().indexOf(columnName);
+      if (ColumnType.STRING.equals(frameReader.signature().getColumnType(columnName).get()) && (row1.get(i) instanceof List || row2.get(i) instanceof List)) {

Review Comment:
   @kgyrtkirk Yeah I agree. We can keep it as it is until we get GlueingPartitioningOperator (I manually verified the MVD queries on my local branch with the GlueingPartitioningOperator changes, all of them error out properly).
   
   The check in this PR isn't exhaustive, but it atleast works for some kind of queries. Not sure if that's preferable though.



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on PR #17036:
URL: https://github.com/apache/druid/pull/17036#issuecomment-2360580504

   We had some queries like the following giving incorrect results, because the MVDs (List object) were getting converted to string:
   
   ![image](https://github.com/user-attachments/assets/5824d077-38b2-4fbc-a4d1-067b6d91534b)
   
   
   As per offline discussion with @sreemanamala, have opened this PR and have added another check in `RearrangedRowsAndColumns`.
   
   This is a temporary measure and would be removed once we have the GlueingPartitioningOperator approach as mentioned previously.


-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1768098463


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,43 @@
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD_1(String contextName, Map<String, Object> context)

Review Comment:
   I was trying to keep it consistent with the rest of the file. I don't know why it was parameterized from the beginning, maybe it was just kept that way to allow flexibility to extend it in the future if needed.
   
   Have made all tests of MSQWindowTest non-parameterized. Hope that works.



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain closed pull request #17036: MSQ window functions: Reject MVDs during window processing
URL: https://github.com/apache/druid/pull/17036


-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1768078082


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,43 @@
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD_1(String contextName, Map<String, Object> context)

Review Comment:
   what's the point of adding `ParameterizedTest` -s when there is only one parameter?



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1766586880


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java:
##########
@@ -451,6 +453,14 @@ private boolean comparePartitionKeys(ResultRow row1, ResultRow row2, List<String
     int match = 0;
     for (String columnName : partitionColumnNames) {
       int i = frameReader.signature().indexOf(columnName);
+      if (ColumnType.STRING.equals(frameReader.signature().getColumnType(columnName).get()) && (row1.get(i) instanceof List || row2.get(i) instanceof List)) {
+        // special handling to reject MVDs
+        throw new UOE(

Review Comment:
   @sreemanamala I tried that originally, that isn't propagated to the end user in MSQ:
   
   `InvalidInput`:
   <img width="1106" alt="image" src="https://github.com/user-attachments/assets/2f85df04-fa37-4d02-aac5-30813a421741">
   
   `UOE`:
   <img width="1465" alt="image" src="https://github.com/user-attachments/assets/9b027f77-545d-4d29-867b-20934582cee8">
   
   In the case of InvalidInput, the error message gets buried in the stacktrace:
   <details>
   
   ```
   "exceptionStackTrace": "org.apache.druid.java.util.common.ISE: Failed to gather clusterBy statistics for stage[query-9935ebee-5ed5-4f39-99a6-5339f581c7aa_1]\n\tat org.apache.druid.msq.exec.RunWorkOrder$ShufflePipelineBuilder$4.onFailure(RunWorkOrder.java:985)\n\tat com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1119)\n\tat org.apache.druid.java.util.common.concurrent.DirectExecutorService.execute(DirectExecutorService.java:81)\n\tat com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1286)\n\tat com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:1055)\n\tat com.google.common.util.concurrent.AbstractFuture.setException(AbstractFuture.java:807)\n\tat com.google.common.util.concurrent.SettableFuture.setException(SettableFuture.java:55)\n\tat org.apache.druid.frame.processor.RunAllFullyWidget$RunAllFullyRunnable.cleanup(RunAllFullyWidget.java:392)\n\tat org.apache.druid.frame.processor.RunAllFully
 Widget$RunAllFullyRunnable.cleanupIfNoMoreProcessors(RunAllFullyWidget.java:344)\n\tat org.apache.druid.frame.processor.RunAllFullyWidget$RunAllFullyRunnable.access$900(RunAllFullyWidget.java:111)\n\tat org.apache.druid.frame.processor.RunAllFullyWidget$RunAllFullyRunnable$1.onFailure(RunAllFullyWidget.java:328)\n\tat com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1119)\n\tat com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:31)\n\tat com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1286)\n\tat com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:1055)\n\tat com.google.common.util.concurrent.AbstractFuture.setException(AbstractFuture.java:807)\n\tat com.google.common.util.concurrent.SettableFuture.setException(SettableFuture.java:55)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.fail(FrameProcessorExecutor.java:337)\n\tat org.apache.d
 ruid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.run(FrameProcessorExecutor.java:178)\n\tat org.apache.druid.msq.exec.WorkerImpl$2$2.run(WorkerImpl.java:923)\n\tat java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)\n\tat java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)\n\tat java.base/java.util.concurrent.FutureTask.run(FutureTask.java)\n\tat org.apache.druid.query.PrioritizedListenableFutureTask.run(PrioritizedExecutorService.java:259)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:829)\nCaused by: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: org.apache.druid.error.DruidException: Encountered a multi value column [v0]. Window processing does not support MVDs. Consider using UNNEST o
 r MV_TO_ARRAY.\n\tat org.apache.druid.java.util.common.Either.valueOrThrow(Either.java:95)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.runProcessorNow(FrameProcessorExecutor.java:259)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.run(FrameProcessorExecutor.java:138)\n\t... 8 more\nCaused by: java.lang.RuntimeException: java.lang.RuntimeException: org.apache.druid.error.DruidException: Encountered a multi value column [v0]. Window processing does not support MVDs. Consider using UNNEST or MV_TO_ARRAY.\n\tat org.apache.druid.java.util.common.Either.valueOrThrow(Either.java:95)\n\tat org.apache.druid.frame.channel.BlockingQueueFrameChannel$Readable.read(BlockingQueueFrameChannel.java:240)\n\tat org.apache.druid.msq.indexing.processor.KeyStatisticsCollectionProcessor.runIncrementally(KeyStatisticsCollectionProcessor.java:110)\n\tat org.apache.druid.msq.counters.CpuTimeAccumulatingFrameProcessor.runIncrementally(CpuTim
 eAccumulatingFrameProcessor.java:66)\n\tat org.apache.druid.frame.processor.FrameProcessors$1FrameProcessorWithBaggage.runIncrementally(FrameProcessors.java:72)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.runProcessorNow(FrameProcessorExecutor.java:230)\n\t... 9 more\nCaused by: java.lang.RuntimeException: org.apache.druid.error.DruidException: Encountered a multi value column [v0]. Window processing does not support MVDs. Consider using UNNEST or MV_TO_ARRAY.\n\tat org.apache.druid.java.util.common.Either.valueOrThrow(Either.java:95)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.runProcessorNow(FrameProcessorExecutor.java:259)\n\t... 9 more\nCaused by: org.apache.druid.error.DruidException: Encountered a multi value column [v0]. Window processing does not support MVDs. Consider using UNNEST or MV_TO_ARRAY.\n\tat org.apache.druid.error.BaseFailure.makeException(BaseFailure.java:55)\n\tat org.apache.druid.error.Dru
 idException.fromFailure(DruidException.java:157)\n\tat org.apache.druid.error.InvalidInput.exception(InvalidInput.java:35)\n\tat org.apache.druid.error.InvalidInput.exception(InvalidInput.java:30)\n\tat org.apache.druid.msq.querykit.WindowOperatorQueryFrameProcessor.comparePartitionKeys(WindowOperatorQueryFrameProcessor.java:459)\n\tat org.apache.druid.msq.querykit.WindowOperatorQueryFrameProcessor.runIncrementally(WindowOperatorQueryFrameProcessor.java:256)\n\tat org.apache.druid.msq.counters.CpuTimeAccumulatingFrameProcessor.runIncrementally(CpuTimeAccumulatingFrameProcessor.java:66)\n\tat org.apache.druid.frame.processor.FrameProcessors$1FrameProcessorWithBaggage.runIncrementally(FrameProcessors.java:72)\n\tat org.apache.druid.frame.processor.FrameProcessorExecutor$1ExecutorRunnable.runProcessorNow(FrameProcessorExecutor.java:230)\n\t... 9 more\n"
   ```
   
   </details>



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #17036:
URL: https://github.com/apache/druid/pull/17036


-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1756094582


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,23 @@
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD(String contextName, Map<String, Object> context)

Review Comment:
   ## Useless parameter
   
   The parameter 'contextName' is never used.
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/7796)



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on PR #17036:
URL: https://github.com/apache/druid/pull/17036#issuecomment-2352090603

   Based on offline discussion, we can wait for GlueingPartitioningOperator to be used, which would make it return a proper error message. So I'll close this PR.


-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1766604146


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,43 @@
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD_1(String contextName, Map<String, Object> context)
+  {
+    testSelectQuery()
+        .setSql("select cityName, countryName, array_to_mv(array[1,length(cityName)]), "
+                + "row_number() over (partition by  array_to_mv(array[1,length(cityName)]) order by countryName, cityName)\n"
+                + "from wikipedia\n"
+                + "where countryName in ('Austria', 'Republic of Korea') and cityName is not null\n"
+                + "order by 1, 2, 3")
+        .setQueryContext(context)
+        .setExpectedExecutionErrorMatcher(CoreMatchers.allOf(
+            CoreMatchers.instanceOf(ISE.class),
+            ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString(
+                "Encountered a multi value column [v0]. Window processing does not support MVDs. Consider using UNNEST or MV_TO_ARRAY."))
+        ))
+        .verifyExecutionError();
+  }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD_2(String contextName, Map<String, Object> context)

Review Comment:
   ## Useless parameter
   
   The parameter 'contextName' is never used.
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/7911)



##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java:
##########
@@ -2323,4 +2326,43 @@
                      )))
                      .verifyResults();
   }
+
+  @MethodSource("data")
+  @ParameterizedTest(name = "{index}:with context {0}")
+  public void testFailurePartitionByMVD_1(String contextName, Map<String, Object> context)

Review Comment:
   ## Useless parameter
   
   The parameter 'contextName' is never used.
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/7912)



-- 
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] MSQ window functions: Reject MVDs during window processing (druid)

Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #17036:
URL: https://github.com/apache/druid/pull/17036#discussion_r1770810503


##########
processing/src/main/java/org/apache/druid/query/rowsandcols/RearrangedRowsAndColumns.java:
##########
@@ -128,7 +130,16 @@ public boolean isNull(int rowNum)
             @Override
             public Object getObject(int rowNum)
             {
-              return accessor.getObject(pointers[start + rowNum]);
+              Object value = accessor.getObject(pointers[start + rowNum]);
+              if (ColumnType.STRING.equals(getType()) && value instanceof List) {

Review Comment:
   Since we require this check for druid 31, we can live with this "hackery". Once https://github.com/apache/druid/pull/17038 gets merged, please remove this check. 
   cc @Akshat-Jain 



-- 
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