From dd39319bddae5fbb0a6ae0b6f3e2ffbaca039450 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 25 Jun 2024 18:10:43 +0530 Subject: [PATCH 1/4] fix for selector type array --- .../DefaultColumnSelectorFactoryMaker.java | 1 + .../segment/virtual/ExpressionSelectors.java | 5 +++- .../sql/calcite/CalciteWindowQueryTest.java | 28 +++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java b/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java index 3c6d3cc08c9b..2eda694ec101 100644 --- a/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java +++ b/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java @@ -210,6 +210,7 @@ public PassThroughColumnValueSelector( break; case ARRAY: myClazz = List.class; + break; default: throw DruidException.defensive("this class cannot handle type [%s]", columnAccessor.getType()); } diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index ca2cda3e0e14..905b617bf81e 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -496,7 +496,10 @@ static Supplier supplierFromObjectSelector( return () -> { final Object val = selector.getObject(); if (val != null) { - NonnullPair coerced = ExprEval.coerceListToArray((List) val, homogenizeMultiValue); + NonnullPair coerced = ExprEval.coerceListToArray( + Arrays.asList((Object[]) val), + homogenizeMultiValue + ); if (coerced == null) { return null; } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java index cdc4bc9cbf98..9cc7038f7f40 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java @@ -324,6 +324,34 @@ public void testWindowAllBoundsCombination() .run(); } + @Test + public void testWithArrayConcat() + { + testBuilder() + .sql("select countryName, cityName, channel, " + + "array_concat_agg(ARRAY['abc', channel], 10000) over (partition by cityName order by countryName) as c\n" + + "from wikipedia\n" + + "where countryName in ('Austria', 'Republic of Korea') " + + "and (cityName in ('Vienna', 'Seoul') or cityName is null)\n" + + "group by countryName, cityName, channel") + .queryContext(ImmutableMap.of( + PlannerContext.CTX_ENABLE_WINDOW_FNS, true, + QueryContexts.ENABLE_DEBUG, true, + QueryContexts.WINDOWING_STRICT_VALIDATION, false + )) + .expectedResults(ImmutableList.of( + new Object[]{"Austria", null, "#de.wikipedia", "[\"abc\",\"#de.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#en.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#ja.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#ko.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", "Seoul", "#ko.wikipedia", "[\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#de.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#es.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#tr.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"} + )) + .run(); + } + private WindowOperatorQuery getWindowOperatorQuery(List> queries) { assertEquals(1, queries.size()); From 6f99eec866ea890fcc74efa052ffce20fdd57176 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 25 Jun 2024 21:38:44 +0530 Subject: [PATCH 2/4] assign object array class to selector type array --- .../DefaultColumnSelectorFactoryMaker.java | 3 +-- .../segment/virtual/ExpressionSelectors.java | 16 +++++++++++-- .../virtual/ExpressionSelectorsTest.java | 15 ++++++++++++ .../sql/calcite/CalciteWindowQueryTest.java | 23 +++++++++++-------- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java b/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java index 2eda694ec101..9cc87be78e7a 100644 --- a/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java +++ b/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java @@ -41,7 +41,6 @@ import javax.annotation.Nullable; import java.nio.ByteBuffer; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -209,7 +208,7 @@ public PassThroughColumnValueSelector( myClazz = float.class; break; case ARRAY: - myClazz = List.class; + myClazz = Object[].class; break; default: throw DruidException.defensive("this class cannot handle type [%s]", columnAccessor.getType()); diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index 905b617bf81e..49dfe199162b 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -483,7 +483,7 @@ static Supplier supplierFromObjectSelector( return () -> { final Object val = selector.getObject(); if (val instanceof List) { - NonnullPair coerced = ExprEval.coerceListToArray((List) val, homogenizeMultiValue); + NonnullPair coerced = ExprEval.coerceListToArray((List) val, homogenizeMultiValue); if (coerced == null) { return null; } @@ -492,7 +492,7 @@ static Supplier supplierFromObjectSelector( return val; } }; - } else if (clazz.isAssignableFrom(List.class)) { + } else if (clazz.isAssignableFrom(Object[].class)) { return () -> { final Object val = selector.getObject(); if (val != null) { @@ -507,6 +507,18 @@ static Supplier supplierFromObjectSelector( } return null; }; + } else if (clazz.isAssignableFrom(List.class)) { + return () -> { + final Object val = selector.getObject(); + if (val != null) { + NonnullPair coerced = ExprEval.coerceListToArray((List) val, homogenizeMultiValue); + if (coerced == null) { + return null; + } + return coerced.rhs; + } + return null; + }; } else { // No numbers or strings, just pass it through return selector::getObject; diff --git a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionSelectorsTest.java b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionSelectorsTest.java index 8224f24e8954..b6b9713682a8 100644 --- a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionSelectorsTest.java +++ b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionSelectorsTest.java @@ -591,7 +591,22 @@ public void test_supplierFromObjectSelector_onList() settableSupplier.set(ImmutableList.of("1", "2", "3")); Assert.assertArrayEquals(new String[]{"1", "2", "3"}, (Object[]) supplier.get()); + } + @Test + public void test_supplierFromObjectSelector_onArray() + { + final SettableSupplier settableSupplier = new SettableSupplier<>(); + final Supplier supplier = ExpressionSelectors.supplierFromObjectSelector( + objectSelectorFromSupplier(settableSupplier, Object[].class), + true + ); + + Assert.assertNotNull(supplier); + Assert.assertEquals(null, supplier.get()); + + settableSupplier.set(new String[]{"1", "2", "3"}); + Assert.assertArrayEquals(new String[]{"1", "2", "3"}, (Object[]) supplier.get()); } @Test diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java index 9cc7038f7f40..6ab45f9308ad 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java @@ -339,16 +339,19 @@ public void testWithArrayConcat() QueryContexts.ENABLE_DEBUG, true, QueryContexts.WINDOWING_STRICT_VALIDATION, false )) - .expectedResults(ImmutableList.of( - new Object[]{"Austria", null, "#de.wikipedia", "[\"abc\",\"#de.wikipedia\"]"}, - new Object[]{"Republic of Korea", null, "#en.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, - new Object[]{"Republic of Korea", null, "#ja.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, - new Object[]{"Republic of Korea", null, "#ko.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, - new Object[]{"Republic of Korea", "Seoul", "#ko.wikipedia", "[\"abc\",\"#ko.wikipedia\"]"}, - new Object[]{"Austria", "Vienna", "#de.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, - new Object[]{"Austria", "Vienna", "#es.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, - new Object[]{"Austria", "Vienna", "#tr.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"} - )) + .expectedResults( + ResultMatchMode.RELAX_NULLS, + ImmutableList.of( + new Object[]{"Austria", null, "#de.wikipedia", "[\"abc\",\"#de.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#en.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#ja.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", null, "#ko.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#en.wikipedia\",\"abc\",\"#ja.wikipedia\",\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Republic of Korea", "Seoul", "#ko.wikipedia", "[\"abc\",\"#ko.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#de.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#es.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"}, + new Object[]{"Austria", "Vienna", "#tr.wikipedia", "[\"abc\",\"#de.wikipedia\",\"abc\",\"#es.wikipedia\",\"abc\",\"#tr.wikipedia\"]"} + ) + ) .run(); } From 8e5933281139dce13b69db3307fc04e3369bee87 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Fri, 12 Jul 2024 07:02:51 +0530 Subject: [PATCH 3/4] comments --- .../segment/virtual/ExpressionSelectors.java | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index 49dfe199162b..979d213ee411 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -475,8 +475,8 @@ static Supplier supplierFromObjectSelector( } final Class clazz = selector.classOfObject(); - if (Number.class.isAssignableFrom(clazz) || String.class.isAssignableFrom(clazz)) { - // Number, String supported as-is. + if (Number.class.isAssignableFrom(clazz) || String.class.isAssignableFrom(clazz) || Object[].class.isAssignableFrom(clazz)) { + // Number, String, Arrays supported as-is. return selector::getObject; } else if (clazz.isAssignableFrom(Number.class) || clazz.isAssignableFrom(String.class)) { // Might be Numbers and Strings. Use a selector that double-checks. @@ -492,21 +492,6 @@ static Supplier supplierFromObjectSelector( return val; } }; - } else if (clazz.isAssignableFrom(Object[].class)) { - return () -> { - final Object val = selector.getObject(); - if (val != null) { - NonnullPair coerced = ExprEval.coerceListToArray( - Arrays.asList((Object[]) val), - homogenizeMultiValue - ); - if (coerced == null) { - return null; - } - return coerced.rhs; - } - return null; - }; } else if (clazz.isAssignableFrom(List.class)) { return () -> { final Object val = selector.getObject(); From 3d80226b3ffb5060b60052cc2721f9dd60f0ccf1 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 17 Jul 2024 12:42:52 +0530 Subject: [PATCH 4/4] test fix with master merge --- .../org/apache/druid/sql/calcite/CalciteWindowQueryTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java index 4b30c6d2b699..ab20925107f6 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java @@ -332,8 +332,7 @@ public void testWithArrayConcat() + "group by countryName, cityName, channel") .queryContext(ImmutableMap.of( PlannerContext.CTX_ENABLE_WINDOW_FNS, true, - QueryContexts.ENABLE_DEBUG, true, - QueryContexts.WINDOWING_STRICT_VALIDATION, false + QueryContexts.ENABLE_DEBUG, true )) .expectedResults( ResultMatchMode.RELAX_NULLS,