From ea0a10c6166bf6ac04956aace6a183c43e327082 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 11 Mar 2024 17:44:00 -0700 Subject: [PATCH 1/2] fix sql results mixed array and scalar values --- .../druid/sql/calcite/run/SqlResults.java | 3 ++- .../druid/sql/calcite/run/SqlResultsTest.java | 25 ++++++++----------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java b/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java index 649d27a1f3a9..cb27de95818f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java @@ -41,6 +41,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -199,7 +200,7 @@ static Object maybeCoerceArrayToList(Object value, boolean mustCoerce) } return lst; } else if (mustCoerce) { - return null; + return Collections.singletonList(value); } return value; } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java index 7d846f434547..bd2ccddbc71e 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java @@ -62,6 +62,8 @@ public void testCoerceStringArrays() assertCoerceArrayToList(stringList, stringList); assertCoerceArrayToList(stringList, stringArray); assertCoerceArrayToList(stringList, stringArray2); + assertCoerceArrayToList(null, null); + assertCoerceArrayToList(Collections.singletonList("a"), "a"); } @Test @@ -76,6 +78,8 @@ public void testCoerceLongArrays() assertCoerceArrayToList(listWithNull, arrayWithNull); assertCoerceArrayToList(list, list); assertCoerceArrayToList(list, array); + assertCoerceArrayToList(null, null); + assertCoerceArrayToList(Collections.singletonList(1L), 1L); } @Test @@ -90,6 +94,8 @@ public void testCoerceDoubleArrays() assertCoerceArrayToList(listWithNull, arrayWithNull); assertCoerceArrayToList(list, list); assertCoerceArrayToList(list, array); + assertCoerceArrayToList(null, null); + assertCoerceArrayToList(Collections.singletonList(1.1), 1.1); } @Test @@ -104,6 +110,8 @@ public void testCoerceFloatArrays() assertCoerceArrayToList(listWithNull, arrayWithNull); assertCoerceArrayToList(list, list); assertCoerceArrayToList(list, array); + assertCoerceArrayToList(null, null); + assertCoerceArrayToList(Collections.singletonList(1.1f), 1.1f); } @Test @@ -225,11 +233,6 @@ public void testCoerceOfArrayOfPrimitives() Assert.assertEquals("Cannot coerce field [fieldName] from type [Byte Array] to type [BIGINT]", e.getMessage()); } } - @Test - public void testCoerceArrayFails() - { - assertCannotCoerce("xyz", SqlTypeName.ARRAY); - } @Test public void testCoerceUnsupportedType() @@ -238,13 +241,7 @@ public void testCoerceUnsupportedType() } @Test - public void testMustCoerce() - { - Assert.assertNull(SqlResults.maybeCoerceArrayToList("hello", true)); - } - - @Test - public void testMayNotCoerce() + public void testMayNotCoerceList() { Assert.assertEquals("hello", SqlResults.maybeCoerceArrayToList("hello", false)); } @@ -269,9 +266,9 @@ private void assertCannotCoerce(Object toCoerce, SqlTypeName typeName) MatcherAssert.assertThat(e, ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("Cannot coerce"))); } - private static void assertCoerceArrayToList(Object expected, Object toCoerce) + private void assertCoerceArrayToList(Object expected, Object toCoerce) { - Object coerced = SqlResults.maybeCoerceArrayToList(toCoerce, true); + Object coerced = SqlResults.coerce(jsonMapper, DEFAULT_CONTEXT, toCoerce, SqlTypeName.ARRAY, ""); Assert.assertEquals(expected, coerced); } } From 67465eda75e8de742181276dd72a8adcbee04c91 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 11 Mar 2024 23:30:22 -0700 Subject: [PATCH 2/2] simplify --- .../apache/druid/sql/calcite/run/SqlResults.java | 13 +++++-------- .../druid/sql/calcite/run/SqlResultsTest.java | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java b/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java index cb27de95818f..486c23f67c9a 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java @@ -71,7 +71,7 @@ public static Object coerce( } else if (value instanceof Boolean) { coercedValue = String.valueOf(value); } else { - final Object maybeList = maybeCoerceArrayToList(value, false); + final Object maybeList = coerceArrayToList(value, false); // Check if "maybeList" was originally a Collection of some kind, or was able to be coerced to one. // Then Iterate through the collection, coercing each value. Useful for handling multi-value dimensions. @@ -153,10 +153,7 @@ public static Object coerce( // the protobuf jdbc handler prefers lists (it actually can't handle java arrays as sql arrays, only java lists) // the json handler could handle this just fine, but it handles lists as sql arrays as well so just convert // here if needed - coercedValue = maybeCoerceArrayToList(value, true); - if (coercedValue == null) { - throw cannotCoerce(value, sqlTypeName, fieldName); - } + coercedValue = coerceArrayToList(value, true); } } else { throw cannotCoerce(value, sqlTypeName, fieldName); @@ -167,11 +164,11 @@ public static Object coerce( /** * Attempt to coerce a value to {@link List}. If it cannot be coerced, either return the original value (if mustCoerce - * is false) or return null (if mustCoerce is true). + * is false) or return the value as a single element list (if mustCoerce is true). */ @VisibleForTesting @Nullable - static Object maybeCoerceArrayToList(Object value, boolean mustCoerce) + static Object coerceArrayToList(Object value, boolean mustCoerce) { if (value instanceof List) { return value; @@ -185,7 +182,7 @@ static Object maybeCoerceArrayToList(Object value, boolean mustCoerce) final Object[] array = (Object[]) value; final ArrayList lst = new ArrayList<>(array.length); for (Object o : array) { - lst.add(maybeCoerceArrayToList(o, false)); + lst.add(coerceArrayToList(o, false)); } return lst; } else if (value instanceof long[]) { diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java index bd2ccddbc71e..7148adf40fe7 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java @@ -243,7 +243,7 @@ public void testCoerceUnsupportedType() @Test public void testMayNotCoerceList() { - Assert.assertEquals("hello", SqlResults.maybeCoerceArrayToList("hello", false)); + Assert.assertEquals("hello", SqlResults.coerceArrayToList("hello", false)); } private void assertCoerce(Object expected, Object toCoerce, SqlTypeName typeName)