From 29fa34bff59c9737762c01ef69bfc1d2ec6d9dc1 Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Wed, 25 Jan 2023 15:09:17 -0800 Subject: [PATCH 1/6] Expand test framework to allow creating structs with field names. The current transport UDF test framework does not let us create and return structs with field names. To write tests, we use `AbstractStdUDFTest`. This class makes a `FunctionCall` which internally uses `TestTypeUtils`. `TestTypeUtils` creates struct using `TestTypeFactory` and `Row`. Currently `Row` which is used to represent structs in the test framework doesn't have a constructor that lets us create structs with field names. On the other hand, `TestTypeFactory` allows us to do this. So this is a gap in the test framework. This also shows up when paramaters of a `FunctionCall` are resolved in `GenericQueryExecutor::resolveStruct` method. This PR is a step towards allowing test writers to pass a list of string fieldNames along with the data when they want to create structs with data and field names. --- .../linkedin/transport/test/AbstractStdUDFTest.java | 4 ++++ .../transport/test/generic/GenericQueryExecutor.java | 9 ++++++++- .../main/java/com/linkedin/transport/test/spi/Row.java | 10 ++++++++++ .../transport/test/spi/types/TestTypeUtils.java | 8 +++++++- 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java b/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java index 7dbc3d34..f6120c36 100644 --- a/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java +++ b/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java @@ -88,6 +88,10 @@ protected static Map map(Object... args) { return dataMap; } + protected static Row rowWithFieldNames(List fieldNames, List args) { + return new Row(fieldNames, args); + } + /** * Creates a row from the provided elements to pass to the test framework */ diff --git a/transportable-udfs-test/transportable-udfs-test-generic/src/main/java/com/linkedin/transport/test/generic/GenericQueryExecutor.java b/transportable-udfs-test/transportable-udfs-test-generic/src/main/java/com/linkedin/transport/test/generic/GenericQueryExecutor.java index 0c4d17dd..5982095a 100644 --- a/transportable-udfs-test/transportable-udfs-test-generic/src/main/java/com/linkedin/transport/test/generic/GenericQueryExecutor.java +++ b/transportable-udfs-test/transportable-udfs-test-generic/src/main/java/com/linkedin/transport/test/generic/GenericQueryExecutor.java @@ -116,12 +116,19 @@ private Pair resolveMap(Map map, TestType keyT private Pair resolveStruct(Row struct, List fieldTypes) { List resolvedFieldTypes = new ArrayList<>(); + List resolvedFieldNames = new ArrayList<>(); List resolvedFields = new ArrayList<>(); IntStream.range(0, fieldTypes.size()).forEach(idx -> { Pair resolvedField = resolveParameter(struct.getFields().get(idx), fieldTypes.get(idx)); + if (struct.getFieldNames() != null) { + resolvedFieldNames.add(struct.getFieldNames().get(idx)); + } resolvedFieldTypes.add(resolvedField.getLeft()); resolvedFields.add(resolvedField.getRight()); }); - return Pair.of(TestTypeFactory.struct(resolvedFieldTypes), new Row(resolvedFields)); + if (resolvedFieldNames.isEmpty()) { + return Pair.of(TestTypeFactory.struct(resolvedFieldTypes), new Row(resolvedFields)); + } + return Pair.of(TestTypeFactory.struct(resolvedFieldNames, resolvedFieldTypes), new Row(resolvedFieldNames, resolvedFields)); } } diff --git a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/Row.java b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/Row.java index 66046364..c42f0bfd 100644 --- a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/Row.java +++ b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/Row.java @@ -12,15 +12,25 @@ public class Row { private final List _fields; + private List _fieldNames; public Row(List fields) { _fields = fields; } + public Row(List fieldNames, List fields) { + _fieldNames = fieldNames; + _fields = fields; + } + public List getFields() { return _fields; } + public List getFieldNames() { + return _fieldNames; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/types/TestTypeUtils.java b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/types/TestTypeUtils.java index 670e4aad..b4d91956 100644 --- a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/types/TestTypeUtils.java +++ b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/types/TestTypeUtils.java @@ -43,8 +43,14 @@ public static TestType inferTypeFromData(Object data) { return TestTypeFactory.map(inferCollectionTypeFromData(map.keySet(), "map keys"), inferCollectionTypeFromData(map.values(), "map values")); } else if (data instanceof Row) { + Row row = (Row) data; + if (row.getFieldNames() == null) { + return TestTypeFactory.struct( + row.getFields().stream().map(TestTypeUtils::inferTypeFromData).collect(Collectors.toList())); + } return TestTypeFactory.struct( - ((Row) data).getFields().stream().map(TestTypeUtils::inferTypeFromData).collect(Collectors.toList())); + row.getFieldNames(), + row.getFields().stream().map(TestTypeUtils::inferTypeFromData).collect(Collectors.toList())); } else if (data instanceof FunctionCall) { return TestTypeFactory.UNKNOWN_TEST_TYPE; } else { From 1b2b6ab1cf86f3dc97e89e8526d6a65d3de678d6 Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Thu, 26 Jan 2023 15:37:53 -0800 Subject: [PATCH 2/6] Add a unit test and address review comments --- .../examples/DeleteFieldOfAStruct.java | 62 +++++++++++++++++++ .../examples/TestDeleteFieldOfAStruct.java | 43 +++++++++++++ .../transport/test/AbstractStdUDFTest.java | 12 ++-- 3 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java create mode 100644 transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java new file mode 100644 index 00000000..4bf2270b --- /dev/null +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java @@ -0,0 +1,62 @@ +package com.linkedin.transport.examples; + +import com.google.common.collect.ImmutableList; +import com.linkedin.transport.api.data.StdData; +import com.linkedin.transport.api.data.StdString; +import com.linkedin.transport.api.data.StdStruct; +import com.linkedin.transport.api.udf.StdUDF2; +import com.linkedin.transport.api.udf.TopLevelStdUDF; +import java.util.List; + + +public class DeleteFieldOfAStruct extends StdUDF2 implements TopLevelStdUDF { + + /** + * + * @param input struct from which the field is to be removed + * @param fieldName name of field to be removed. If no such field, don't do anything. + * @return struct with field removed + */ + @Override + public StdData eval(StdData input, StdString fieldName) { + if (input instanceof StdStruct) { + StdStruct inputAsStruct = ((StdStruct) input); + StdData field = inputAsStruct.getField(fieldName.get()); + if (field == null) { + // no field with the matching name, don't do anything + return input; + } else { + // Replaced with empty string. + inputAsStruct.setField(fieldName.get(), getStdFactory().createString("")); + } + } else { + throw new RuntimeException("Works only with a struct"); + } + return input; + } + + + @Override + public List getInputParameterSignatures() { + return ImmutableList.of( + "row(varchar, varchar)", + "varchar" + ); + } + + @Override + public String getOutputParameterSignature() { + return "row(varchar, varchar)"; + } + + @Override + public String getFunctionName() { + return "deleteFieldOfAStruct"; + } + + @Override + public String getFunctionDescription() { + return "changes the content of the field called 'fieldName' to empty string"; + } + +} diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java new file mode 100644 index 00000000..efa0fab3 --- /dev/null +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java @@ -0,0 +1,43 @@ +package com.linkedin.transport.examples; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.linkedin.transport.api.udf.StdUDF; +import com.linkedin.transport.api.udf.TopLevelStdUDF; +import com.linkedin.transport.test.AbstractStdUDFTest; +import com.linkedin.transport.test.spi.Row; +import com.linkedin.transport.test.spi.StdTester; +import java.util.List; +import java.util.Map; +import org.testng.annotations.Test; + + +public class TestDeleteFieldOfAStruct extends AbstractStdUDFTest { + + @Override + protected Map, List>> getTopLevelStdUDFClassesAndImplementations() { + return ImmutableMap.of(DeleteFieldOfAStruct.class, ImmutableList.of(DeleteFieldOfAStruct.class)); + } + + @Test + public void testDeleteFieldOfAStructFunction() { + StdTester tester = getTester(); + Row data = rowWithFieldNames( + ImmutableList.of("firstName","lastName"), + ImmutableList.of("foo","bar")); + Row dataAfterDeleteFirstName = rowWithFieldNames( + ImmutableList.of("firstName","lastName"), + ImmutableList.of("","bar")); // <- value corresponding to field name "firstName" has been removed + Row dataAfterDeleteLastName = rowWithFieldNames( + ImmutableList.of("firstName","lastName"), + ImmutableList.of("foo","")); // <- value corresponding to field name "lastName" has been removed + tester.check( + functionCall("deleteFieldOfAStruct", data, "firstName"), + dataAfterDeleteFirstName, + "row(varchar, varchar)"); + tester.check( + functionCall("deleteFieldOfAStruct", data, "lastName"), + dataAfterDeleteLastName, + "row(varchar, varchar)"); + } +} diff --git a/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java b/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java index f6120c36..ee7d0681 100644 --- a/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java +++ b/transportable-udfs-test/transportable-udfs-test-api/src/main/java/com/linkedin/transport/test/AbstractStdUDFTest.java @@ -88,10 +88,6 @@ protected static Map map(Object... args) { return dataMap; } - protected static Row rowWithFieldNames(List fieldNames, List args) { - return new Row(fieldNames, args); - } - /** * Creates a row from the provided elements to pass to the test framework */ @@ -99,6 +95,14 @@ protected static Row row(Object... args) { return new Row(Arrays.asList(args)); } + /** + * Creates a row from the provided elements with the corresponding field names + * to pass to the test framework + */ + protected static Row rowWithFieldNames(List fieldNames, List args) { + return new Row(fieldNames, args); + } + /** * Creates a string containing the absolute path from the provided relative path of the resource to pass to the * test framework From 3124386d368eaec83189a75cb01fd14abac7c370 Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Thu, 26 Jan 2023 16:02:21 -0800 Subject: [PATCH 3/6] Fix some indentation --- .../com/linkedin/transport/examples/DeleteFieldOfAStruct.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java index 4bf2270b..658e9e22 100644 --- a/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java @@ -26,8 +26,8 @@ public StdData eval(StdData input, StdString fieldName) { // no field with the matching name, don't do anything return input; } else { - // Replaced with empty string. - inputAsStruct.setField(fieldName.get(), getStdFactory().createString("")); + // Replaced with empty string. + inputAsStruct.setField(fieldName.get(), getStdFactory().createString("")); } } else { throw new RuntimeException("Works only with a struct"); From 85415670053d999d732baaf42b81625314d08f48 Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Thu, 26 Jan 2023 16:12:59 -0800 Subject: [PATCH 4/6] Add license header --- .../linkedin/transport/examples/DeleteFieldOfAStruct.java | 5 +++++ .../transport/examples/TestDeleteFieldOfAStruct.java | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java index 658e9e22..9a8aaa8f 100644 --- a/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/main/java/com/linkedin/transport/examples/DeleteFieldOfAStruct.java @@ -1,3 +1,8 @@ +/** + * Copyright 2023 LinkedIn Corporation. All rights reserved. + * Licensed under the BSD-2 Clause license. + * See LICENSE in the project root for license information. + */ package com.linkedin.transport.examples; import com.google.common.collect.ImmutableList; diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java index efa0fab3..9603e0de 100644 --- a/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java @@ -1,3 +1,8 @@ +/** + * Copyright 2023 LinkedIn Corporation. All rights reserved. + * Licensed under the BSD-2 Clause license. + * See LICENSE in the project root for license information. + */ package com.linkedin.transport.examples; import com.google.common.collect.ImmutableList; From f15cc044192e41c0fbb66b3f76f6b2bc8423c5bc Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Thu, 26 Jan 2023 16:24:47 -0800 Subject: [PATCH 5/6] Fix checkStyle --- .../transport/examples/TestDeleteFieldOfAStruct.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java index 9603e0de..f96a9a21 100644 --- a/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java +++ b/transportable-udfs-examples/transportable-udfs-example-udfs/src/test/java/com/linkedin/transport/examples/TestDeleteFieldOfAStruct.java @@ -28,14 +28,14 @@ protected Map, List>> ge public void testDeleteFieldOfAStructFunction() { StdTester tester = getTester(); Row data = rowWithFieldNames( - ImmutableList.of("firstName","lastName"), - ImmutableList.of("foo","bar")); + ImmutableList.of("firstName", "lastName"), + ImmutableList.of("foo", "bar")); Row dataAfterDeleteFirstName = rowWithFieldNames( - ImmutableList.of("firstName","lastName"), - ImmutableList.of("","bar")); // <- value corresponding to field name "firstName" has been removed + ImmutableList.of("firstName", "lastName"), + ImmutableList.of("", "bar")); // <- value corresponding to field name "firstName" has been removed Row dataAfterDeleteLastName = rowWithFieldNames( - ImmutableList.of("firstName","lastName"), - ImmutableList.of("foo","")); // <- value corresponding to field name "lastName" has been removed + ImmutableList.of("firstName", "lastName"), + ImmutableList.of("foo", "")); // <- value corresponding to field name "lastName" has been removed tester.check( functionCall("deleteFieldOfAStruct", data, "firstName"), dataAfterDeleteFirstName, From cd841d97001d7918759362ba90bb9112d23c7c70 Mon Sep 17 00:00:00 2001 From: Mohit Verma Date: Fri, 27 Jan 2023 15:07:41 -0800 Subject: [PATCH 6/6] Use `named_struct` to create named structs in test framework's SqlFunctionCallGenerator --- .../test/spi/SqlFunctionCallGenerator.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/SqlFunctionCallGenerator.java b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/SqlFunctionCallGenerator.java index 98914842..1f931e1d 100644 --- a/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/SqlFunctionCallGenerator.java +++ b/transportable-udfs-test/transportable-udfs-test-spi/src/main/java/com/linkedin/transport/test/spi/SqlFunctionCallGenerator.java @@ -134,7 +134,21 @@ default String getMapArgumentString(Map map, TestType mapKeyType * Returns a SQL string of the format {@code STRUCT(f1, f2, f3, ...)} representing a struct literal */ default String getStructArgumentString(Row struct, List structFieldTypes) { + List structFieldNames = struct.getFieldNames(); List structFields = struct.getFields(); + if (structFieldNames != null) { + StringBuilder argStr = new StringBuilder(); + argStr.append("NAMED_STRUCT("); + for (int idx = 0; idx < structFields.size(); idx++) { + argStr.append(getStringArgumentString(structFieldNames.get(idx))); + argStr.append(","); + argStr.append(getFunctionCallArgumentString(structFields.get(idx), structFieldTypes.get(idx))); + argStr.append(","); + } + argStr.deleteCharAt(argStr.length() - 1); + argStr.append(")"); + return argStr.toString(); + } return "STRUCT" + "(" + IntStream.range(0, structFields.size()) .mapToObj(idx -> getFunctionCallArgumentString(structFields.get(idx), structFieldTypes.get(idx))) .collect(Collectors.joining(", ")) + ")";