[
https://issues.apache.org/jira/browse/BEAM-4573?focusedWorklogId=112994&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-112994
]
ASF GitHub Bot logged work on BEAM-4573:
----------------------------------------
Author: ASF GitHub Bot
Created on: 19/Jun/18 03:49
Start Date: 19/Jun/18 03:49
Worklog Time Spent: 10m
Work Description: kennknowles closed pull request #5667: [BEAM-BEAM-4573]
Add framework for exhaustively checking SQL operator support
URL: https://github.com/apache/beam/pull/5667
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java
b/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java
index 194e7503363..5cf0073d97d 100644
---
a/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java
+++
b/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java
@@ -107,7 +107,7 @@ public void registerUdaf(String functionName,
Combine.CombineFn combineFn) {
try {
return planner.convertToBeamRel(query).toPTransform();
} catch (ValidationException | RelConversionException | SqlParseException
e) {
- throw new ParseException("Unable to parse query", e);
+ throw new ParseException(String.format("Unable to parse query %s",
query), e);
}
}
diff --git
a/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java
b/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java
new file mode 100644
index 00000000000..9ea66899a89
--- /dev/null
+++
b/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Repeatable;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import
org.apache.beam.sdk.extensions.sql.integrationtest.BeamSqlBuiltinFunctionsIntegrationTestBase;
+import org.apache.beam.sdk.schemas.Schema;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.junit.Ignore;
+import org.junit.Test;
+
+/**
+ * DSL compliance tests for the row-level operators of {@link
+ * org.apache.calcite.sql.fun.SqlStdOperatorTable}.
+ */
+public class BeamSqlDslSqlStdOperatorsTest extends
BeamSqlBuiltinFunctionsIntegrationTestBase {
+
+ /** Calcite operators are identified by name and kind. */
+ @AutoValue
+ abstract static class SqlOperatorId {
+ abstract String name();
+
+ abstract SqlKind kind();
+ }
+
+ private static SqlOperatorId sqlOperatorId(String nameAndKind) {
+ return sqlOperatorId(nameAndKind, SqlKind.valueOf(nameAndKind));
+ }
+
+ private static SqlOperatorId sqlOperatorId(String name, SqlKind kind) {
+ return new AutoValue_BeamSqlDslSqlStdOperatorsTest_SqlOperatorId(name,
kind);
+ }
+
+ private static SqlOperatorId sqlOperatorId(SqlOperatorTest annotation) {
+ return sqlOperatorId(annotation.name(),
SqlKind.valueOf(annotation.kind()));
+ }
+
+ private static final List<SqlOperatorId> NON_ROW_OPERATORS =
+ ImmutableList.of(
+ sqlOperatorId("UNION"),
+ sqlOperatorId("UNION ALL", SqlKind.UNION),
+ sqlOperatorId("EXCEPT"),
+ sqlOperatorId("EXCEPT ALL", SqlKind.EXCEPT),
+ sqlOperatorId("INTERSECT"),
+ sqlOperatorId("INTERSECT ALL", SqlKind.INTERSECT));
+
+ /**
+ * LEGACY ADAPTER - DO NOT USE DIRECTLY. Use {@code
getAnnotationsByType(SqlOperatorTest.class)},
+ * a more reliable method for retrieving repeated annotations.
+ *
+ * <p>This is a virtual annotation that is only present when there are more
than one {@link
+ * SqlOperatorTest} annotations. When there is just one {@link
SqlOperatorTest} annotation the
+ * proxying is not in place.
+ */
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD})
+ private @interface SqlOperatorTests {
+ SqlOperatorTest[] value();
+ }
+
+ /**
+ * Annotation that declares a test method has the tests for the {@link
SqlOperatorId}.
+ *
+ * <p>It is almost identical to {@link SqlOperatorId} but complex types
cannot be part of
+ * annotations and there are minor benefits to having a non-annotation class
for passing around
+ * the ids.
+ */
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD})
+ @Repeatable(SqlOperatorTests.class)
+ private @interface SqlOperatorTest {
+ String name();
+
+ String kind();
+ }
+
+ private Set<SqlOperatorId> getTestedOperators() {
+ Set<SqlOperatorId> testedOperators = new HashSet<>();
+
+ for (Method method : getClass().getMethods()) {
+ testedOperators.addAll(
+ Arrays.stream(method.getAnnotationsByType(SqlOperatorTest.class))
+ .map(annotation -> sqlOperatorId(annotation))
+ .collect(Collectors.toList()));
+ }
+
+ return testedOperators;
+ }
+
+ private Set<SqlOperatorId> getDeclaredOperators() {
+ Set<SqlOperatorId> declaredOperators = new HashSet<>();
+
+ declaredOperators.addAll(
+ SqlStdOperatorTable.instance()
+ .getOperatorList()
+ .stream()
+ .map(operator -> sqlOperatorId(operator.getName(),
operator.getKind()))
+ .collect(Collectors.toList()));
+
+ return declaredOperators;
+ }
+
+ private final Comparator<SqlOperatorId> orderByNameThenKind =
+ Ordering.compound(
+ ImmutableList.of(
+ Comparator.comparing((SqlOperatorId operator) ->
operator.name()),
+ Comparator.comparing((SqlOperatorId operator) ->
operator.kind())));
+
+ /** Smoke test that the whitelists and utility functions actually work. */
+ @Test
+ @SqlOperatorTest(name = "CARDINALITY", kind = "OTHER_FUNCTION")
+ public void testAnnotationEquality() throws Exception {
+ Method thisMethod = getClass().getMethod("testAnnotationEquality");
+ SqlOperatorTest sqlOperatorTest =
thisMethod.getAnnotationsByType(SqlOperatorTest.class)[0];
+ assertThat(
+ sqlOperatorId(sqlOperatorTest),
+ equalTo(sqlOperatorId("CARDINALITY", SqlKind.OTHER_FUNCTION)));
+ }
+
+ /**
+ * Tests that all operators in {@link SqlStdOperatorTable} have DSL-level
tests. Operators in the
+ * table are uniquely identified by (kind, name).
+ */
+ @Ignore("https://issues.apache.org/jira/browse/BEAM-4573";)
+ @Test
+ public void testThatAllOperatorsAreTested() {
+ Set<SqlOperatorId> untestedOperators = new HashSet<>();
+ untestedOperators.addAll(getDeclaredOperators());
+
+ // Query-level operators need their own larger test suites
+ untestedOperators.removeAll(NON_ROW_OPERATORS);
+ untestedOperators.removeAll(getTestedOperators());
+
+ if (!untestedOperators.isEmpty()) {
+ // Sorting is just to make failures more readable until we have 100%
coverage
+ List<SqlOperatorId> untestedList = Lists.newArrayList(untestedOperators);
+ untestedList.sort(orderByNameThenKind);
+ fail("No tests declared for operators:\n\t" +
Joiner.on("\n\t").join(untestedList));
+ }
+ }
+
+ /**
+ * Tests that we didn't typo an annotation, that all things we claim to test
are real operators.
+ */
+ @Test
+ public void testThatOperatorsExist() {
+ Set<SqlOperatorId> undeclaredOperators = new HashSet<>();
+ undeclaredOperators.addAll(getTestedOperators());
+ undeclaredOperators.removeAll(getDeclaredOperators());
+
+ if (!undeclaredOperators.isEmpty()) {
+ // Sorting is just to make failures more readable
+ List<SqlOperatorId> undeclaredList =
Lists.newArrayList(undeclaredOperators);
+ undeclaredList.sort(orderByNameThenKind);
+ fail(
+ "Tests declared for nonexistent operators:\n\t" +
Joiner.on("\n\t").join(undeclaredList));
+ }
+ }
+
+ @Test
+ @SqlOperatorTest(name = "CHARACTER_LENGTH", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "CHAR_LENGTH", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "INITCAP", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "LOWER", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "POSITION", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "OVERLAY", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "SUBSTRING", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "TRIM", kind = "TRIM")
+ @SqlOperatorTest(name = "UPPER", kind = "OTHER_FUNCTION")
+ public void testStringFunctions() {
+ ExpressionChecker checker =
+ new ExpressionChecker()
+ .addExpr("'hello' || ' world'", "hello world")
+ .addExpr("CHAR_LENGTH('hello')", 5)
+ .addExpr("CHARACTER_LENGTH('hello')", 5)
+ .addExpr("INITCAP('hello world')", "Hello World")
+ .addExpr("LOWER('HELLO')", "hello")
+ .addExpr("POSITION('world' IN 'helloworld')", 6)
+ .addExpr("POSITION('world' IN 'helloworldworld' FROM 7)", 11)
+ .addExpr("POSITION('world' IN 'hello')", 0)
+ .addExpr("TRIM(' hello ')", "hello")
+ .addExpr("TRIM(LEADING 'eh' FROM 'hehe__hehe')", "__hehe")
+ .addExpr("TRIM(TRAILING 'eh' FROM 'hehe__hehe')", "hehe__")
+ .addExpr("TRIM(BOTH 'eh' FROM 'hehe__hehe')", "__")
+ .addExpr("TRIM(BOTH ' ' FROM ' hello ')", "hello")
+ .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3)",
"w3resou3rce")
+ .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 4)",
"w3resou33rce")
+ .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 5)",
"w3resou3rce")
+ .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 7)",
"w3resouce")
+ .addExpr("SUBSTRING('hello' FROM 2)", "ello")
+ .addExpr("SUBSTRING('hello' FROM -1)", "o")
+ .addExpr("SUBSTRING('hello' FROM 2 FOR 2)", "el")
+ .addExpr("SUBSTRING('hello' FROM 2 FOR 100)", "ello")
+ .addExpr("SUBSTRING('hello' FROM -3 for 2)", "ll")
+ .addExpr("UPPER('hello')", "HELLO");
+
+ checker.buildRunAndCheck();
+ }
+
+ @Test
+ @SqlOperatorTest(name = "ARRAY", kind = "ARRAY_VALUE_CONSTRUCTOR")
+ @SqlOperatorTest(name = "CARDINALITY", kind = "OTHER_FUNCTION")
+ @SqlOperatorTest(name = "ELEMENT", kind = "OTHER_FUNCTION")
+ public void testArrayFunctions() {
+ ExpressionChecker checker =
+ new ExpressionChecker()
+ // Calcite throws a parse error on this syntax for an
empty array
+ // .addExpr(
+ // "ARRAY []", ImmutableList.of(),
+ // Schema.FieldType.array(Schema.FieldType.BOOLEAN))
+ .addExpr(
+ "ARRAY ['a', 'b']",
+ ImmutableList.of("a", "b"),
+ Schema.FieldType.array(Schema.FieldType.STRING))
+ .addExpr("CARDINALITY(ARRAY ['a', 'b', 'c'])", 3)
+ .addExpr("ELEMENT(ARRAY [1])", 1);
+
+ checker.buildRunAndCheck();
+ }
+}
diff --git
a/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslStringOperatorsTest.java
b/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslStringOperatorsTest.java
deleted file mode 100644
index b50454b2286..00000000000
---
a/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslStringOperatorsTest.java
+++ /dev/null
@@ -1,55 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.beam.sdk.extensions.sql;
-
-import
org.apache.beam.sdk.extensions.sql.integrationtest.BeamSqlBuiltinFunctionsIntegrationTestBase;
-import org.junit.Test;
-
-/** Integration test for string functions. */
-public class BeamSqlDslStringOperatorsTest extends
BeamSqlBuiltinFunctionsIntegrationTestBase {
- @Test
- public void testStringFunctions() throws Exception {
- ExpressionChecker checker =
- new ExpressionChecker()
- .addExpr("'hello' || ' world'", "hello world")
- .addExpr("CHAR_LENGTH('hello')", 5)
- .addExpr("CHARACTER_LENGTH('hello')", 5)
- .addExpr("INITCAP('hello world')", "Hello World")
- .addExpr("LOWER('HELLO')", "hello")
- .addExpr("POSITION('world' IN 'helloworld')", 6)
- .addExpr("POSITION('world' IN 'helloworldworld' FROM 7)", 11)
- .addExpr("POSITION('world' IN 'hello')", 0)
- .addExpr("TRIM(' hello ')", "hello")
- .addExpr("TRIM(LEADING 'eh' FROM 'hehe__hehe')", "__hehe")
- .addExpr("TRIM(TRAILING 'eh' FROM 'hehe__hehe')", "hehe__")
- .addExpr("TRIM(BOTH 'eh' FROM 'hehe__hehe')", "__")
- .addExpr("TRIM(BOTH ' ' FROM ' hello ')", "hello")
- .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3)",
"w3resou3rce")
- .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 4)",
"w3resou33rce")
- .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 5)",
"w3resou3rce")
- .addExpr("OVERLAY('w3333333rce' PLACING 'resou' FROM 3 FOR 7)",
"w3resouce")
- .addExpr("SUBSTRING('hello' FROM 2)", "ello")
- .addExpr("SUBSTRING('hello' FROM -1)", "o")
- .addExpr("SUBSTRING('hello' FROM 2 FOR 2)", "el")
- .addExpr("SUBSTRING('hello' FROM 2 FOR 100)", "ello")
- .addExpr("SUBSTRING('hello' FROM -3 for 2)", "ll")
- .addExpr("UPPER('hello')", "HELLO");
-
- checker.buildRunAndCheck();
- }
-}
diff --git
a/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/integrationtest/BeamSqlBuiltinFunctionsIntegrationTestBase.java
b/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/integrationtest/BeamSqlBuiltinFunctionsIntegrationTestBase.java
index 9990a59e66e..3cd9ebfb93a 100644
---
a/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/integrationtest/BeamSqlBuiltinFunctionsIntegrationTestBase.java
+++
b/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/integrationtest/BeamSqlBuiltinFunctionsIntegrationTestBase.java
@@ -18,6 +18,9 @@
package org.apache.beam.sdk.extensions.sql.integrationtest;
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import java.math.BigDecimal;
import java.util.ArrayList;
@@ -33,14 +36,14 @@
import org.apache.beam.sdk.testing.TestPipeline;
import org.apache.beam.sdk.values.PCollection;
import org.apache.beam.sdk.values.Row;
-import org.apache.calcite.util.Pair;
import org.joda.time.DateTime;
import org.joda.time.format.DateTimeFormat;
import org.junit.Rule;
/** Base class for all built-in functions integration tests. */
public class BeamSqlBuiltinFunctionsIntegrationTestBase {
- private static final Map<Class, TypeName> JAVA_CLASS_TO_FIELDTYPE =
+
+ private static final Map<Class, TypeName> JAVA_CLASS_TO_TYPENAME =
ImmutableMap.<Class, TypeName>builder()
.put(Byte.class, TypeName.BYTE)
.put(Short.class, TypeName.INT16)
@@ -99,6 +102,22 @@ protected static DateTime parseDate(String str) {
return DateTimeFormat.forPattern("yyyy-MM-dd
HH:mm:ss").withZoneUTC().parseDateTime(str);
}
+ @AutoValue
+ abstract static class ExpressionTestCase {
+
+ private static ExpressionTestCase of(
+ String sqlExpr, Object expectedResult, FieldType resultFieldType) {
+ return new
AutoValue_BeamSqlBuiltinFunctionsIntegrationTestBase_ExpressionTestCase(
+ sqlExpr, expectedResult, resultFieldType);
+ }
+
+ abstract String sqlExpr();
+
+ abstract Object expectedResult();
+
+ abstract FieldType resultFieldType();
+ }
+
/**
* Helper class to make write integration test for built-in functions easier.
*
@@ -115,10 +134,22 @@ protected static DateTime parseDate(String str) {
* }</pre>
*/
public class ExpressionChecker {
- private transient List<Pair<String, Object>> exps = new ArrayList<>();
+ private transient List<ExpressionTestCase> exps = new ArrayList<>();
public ExpressionChecker addExpr(String expression, Object expectedValue) {
- exps.add(Pair.of(expression, expectedValue));
+ // Because of erasure, we can only automatically infer non-parameterized
types
+ TypeName resultTypeName =
JAVA_CLASS_TO_TYPENAME.get(expectedValue.getClass());
+ checkArgument(
+ resultTypeName != null,
+ "Could not infer a Beam type for %s."
+ + " Parameterized types must be provided explicitly.");
+ addExpr(expression, expectedValue, FieldType.of(resultTypeName));
+ return this;
+ }
+
+ public ExpressionChecker addExpr(
+ String expression, Object expectedValue, FieldType resultFieldType) {
+ exps.add(ExpressionTestCase.of(expression, expectedValue,
resultFieldType));
return this;
}
@@ -126,15 +157,11 @@ public ExpressionChecker addExpr(String expression,
Object expectedValue) {
public void buildRunAndCheck() {
PCollection<Row> inputCollection = getTestPCollection();
- for (Pair<String, Object> testCase : exps) {
- String expression = testCase.left;
- Object expectedValue = testCase.right;
+ for (ExpressionTestCase testCase : exps) {
+ String expression = testCase.sqlExpr();
+ Object expectedValue = testCase.expectedResult();
String sql = String.format("SELECT %s FROM PCOLLECTION", expression);
- Schema schema =
- Schema.builder()
- .addField(
- expression,
FieldType.of(JAVA_CLASS_TO_FIELDTYPE.get(expectedValue.getClass())))
- .build();
+ Schema schema = Schema.builder().addField(expression,
testCase.resultFieldType()).build();
PCollection<Row> output =
inputCollection.apply(testCase.toString(),
SqlTransform.query(sql));
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 112994)
Time Spent: 1h 20m (was: 1h 10m)
> Test all SQL standard operators at the DSL level
> ------------------------------------------------
>
> Key: BEAM-4573
> URL: https://issues.apache.org/jira/browse/BEAM-4573
> Project: Beam
> Issue Type: Improvement
> Components: dsl-sql
> Reporter: Kenneth Knowles
> Assignee: Kenneth Knowles
> Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
