Support UDF in plan generator #3040

pengpeng-lu · 2025-01-14T22:59:12Z

This fixes #3675

normen662

I am putting @alecgrieser on this review as well to make sure the proto layout and dependencies are what we want. I left a bunch of comments. There is some more work required but it's on the right track. I would recommend not ever removing -Werror (not even in your local build). It is useful. Also, please make sure the PR runs through the PRB.

.idea/compiler.xml

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/RecordMetaData.java

...rd-layer-core/src/main/java/com/apple/foundationdb/record/metadata/ScalarValuedFunction.java

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Function.java

...ayer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/MacroFunction.java

...in/java/com/apple/foundationdb/record/query/plan/cascades/KeyExpressionExpansionVisitor.java

...rd-layer-core/src/main/java/com/apple/foundationdb/record/metadata/ScalarValuedFunction.java

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/Key.java

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/RecordMetaData.java

pengpeng-lu · 2025-08-04T23:31:40Z

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/RecordMetaDataBuilder.java

        for (RecordMetaDataProto.PUserDefinedFunction function: metaDataProto.getUserDefinedFunctionsList()) {
-            UserDefinedFunction func = (UserDefinedFunction)PlanSerialization.dispatchFromProtoContainer(new PlanSerializationContext(DefaultPlanSerializationRegistry.INSTANCE,
-                    PlanHashable.CURRENT_FOR_CONTINUATION), function);
+            UserDefinedFunction func = (UserDefinedFunction)PlanSerialization.dispatchFromProtoContainer(serializationContext, function);


serializationContext stores record types that it has "seen" before, for record type that we've "seen", it'll be serialized simply with a reference_id, so we need the "seen" types to deserialize the proto as well.

pengpeng-lu · 2025-08-07T04:22:13Z

...om/apple/foundationdb/relational/recordlayer/query/functions/UserDefinedFunctionCatalog.java

    @Nonnull
    public Optional<? extends CatalogedFunction> lookup(@Nonnull final String functionName, Expressions arguments) {
-        final var functionSupplier = functionsMap.get(functionName);
+        final var functionSupplier = isCaseSensitive ? functionsMap.get(functionName) : functionsMap.get(functionName.toUpperCase(Locale.ROOT));


TemporaryFunctionTest.createTemporaryFunctionCaseSensitivityOption failed so I changed these 2 lines, confused why it worked before?

Can you please revert this line? it is actually incorrect to perform any normalization here. I think the tested you referred to should pass, but if it is not, we'll have to think about the root cause of the bug and solve it there.

so we'll need to handle the case sensitivity somewhere, for example right now if I create a function with lower case, it'll be stored as upper case, but when I call the function with lower case, it'll only look for lower case, so it'll not be able to find the function. In the SqlFunctionCatalogImpl.lookUpBuiltInFunction, we do final var functionValidator = builtInSynonyms.get(name.toLowerCase(Locale.ROOT));. so that's why I think we should put something similar in the UserDefinedFunctionCatalog.lookup`. wdyt?

If you revert all the changes in this file, and run the test TemporaryFunctionTest.createTemporaryFunctionCaseSensitivityOption it should pass if you're working on top of #3511.

Now, I see that the YAML test is failing, this is due to the way you're parsing the function name without consulting the case-sensitivity flag in the plan generator.

@Nonnull @Override public Expression visitUserDefinedScalarFunctionCall(@Nonnull RelationalParser.UserDefinedScalarFunctionCallContext ctx) { final var functionName = ctx.userDefinedScalarFunctionName().getText(); <--------- Expressions arguments = visitFunctionArgs(ctx.functionArgs()); return getDelegate().resolveFunction(functionName, arguments.asList().toArray(new Expression[0])); }

This is not correct, instead you should parse the function name with case-sensitivity checks, this is for example done in visitUid:

fdb-record-layer/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/IdentifierVisitor.java

Lines 73 to 81 in 6c0597e

@Nonnull

@Override

public Identifier visitUid(@Nonnull RelationalParser.UidContext uidContext) {

if (uidContext.simpleId() != null) {

return visitSimpleId(uidContext.simpleId());

} else {

return Identifier.of(getDelegate().normalizeString(uidContext.getText()));

}

}

So, you can for example change the parser rules and make userDefinedScalarFunctionName delegate to uid:

userDefinedScalarFunctionName : uid ;

Then, in visitUserDefinedScalarFunctionCall make it delegate to to visitUid:

@Nonnull @Override public Expression visitUserDefinedScalarFunctionCall(@Nonnull RelationalParser.UserDefinedScalarFunctionCallContext ctx) { final var functionName = visitUid(ctx.userDefinedScalarFunctionName().uid()); Expressions arguments = visitFunctionArgs(ctx.functionArgs()); return getDelegate().resolveFunction(functionName.getName(), arguments.asList().toArray(new Expression[0])); }

That way, parsing the function name respects whatever case-sensitivity option set in the SQL session. The YAML test should pass.

(I haven't checked whether this is causing any ambiguity problems in the parser though, so you might want to double-check before refactoring according to the above).

pengpeng-lu · 2025-08-07T04:22:42Z

...re/src/test/java/com/apple/foundationdb/relational/recordlayer/query/StandardQueryTests.java

        }
    }

+    @Test


working on moving these tests to yaml test.

would be nice to make the YAML tests part of this PR as well :-)

yes, moved to yaml :)

should we keep the tests or remove them now that we have equivalent tests in YAML.

fdb-relational-core/src/main/antlr/RelationalParser.g4

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/RecordMetaDataBuilder.java

hatyo · 2025-08-21T17:56:02Z

...al-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/AstNormalizer.java

            // during plan generation of the temporary function. The literals and combined with query literals and provided
            // for the execution of a (cached) physical plan.
-            final var compiledFunction = recordLayerRoutine.getCompilableSqlFunctionSupplier().apply(caseSensitive);
+            final var compiledFunction = (CompiledSqlFunction)recordLayerRoutine.getUserDefinedFunctionSupplier().apply(caseSensitive);


nothing is preventing us from creating a temporary marco function, but if the user does so, I suppose the cast above will throw?

...core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/SemanticAnalyzer.java

hatyo · 2025-08-21T18:09:32Z

...ional-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Identifier.java

    }

+    @Nonnull
+    public List<String> removePrefix(@Nonnull Identifier prefix) {


This can be removed once the already existing resolver is used to resolve the nested field which is the result of the macro function.

...re/src/test/java/com/apple/foundationdb/relational/recordlayer/query/AstNormalizerTests.java

hatyo · 2025-08-21T18:11:17Z

...re/src/test/java/com/apple/foundationdb/relational/recordlayer/query/StandardQueryTests.java

        }
    }

+    @Test


would be nice to make the YAML tests part of this PR as well :-)

...ain/java/com/apple/foundationdb/relational/recordlayer/query/visitors/DelegatingVisitor.java

hatyo

LGTM

normen662 · 2025-10-14T19:40:57Z

...c/main/java/com/apple/foundationdb/record/query/plan/cascades/UserDefinedScalarFunction.java

@@ -1,5 +1,5 @@
 /*
- * MacroFunction.java
+ * UserDefinedScalarFunction.java


This is a scalar function:

CREATE FUNCTION FOO(x INT) AS SELECT COUNT(a) FROM T WHERE x = 10

but it's not what this PR is about. I don't like the new name and vote for a name that reflects that the function is

scalar

it also expands in place

makes it clear it's not the kind of function I listed above.

normen662 · 2025-10-20T08:43:20Z

...rc/main/java/com/apple/foundationdb/record/query/plan/cascades/UserDefinedMacroFunction.java

@@ -38,15 +38,15 @@
 import java.util.stream.Collectors;

 /**
- * MacroFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.
+ * UserDefinedScalarFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.


There are user-defined scalar functions that are not covered by this kind of function, i.e:

CREATE FUNCTION foo(p int) AS SELECT count(a) FROM T GROUP BY x WHERE x = p

These are regular SQL-bodied functions that do provably (or only at-runtime provably) return exactly one value.

I liked the name MacroFunction as it encapsulated the drop in/replace logic of these mini-functions. Can we rename it back? Or something else that clearly describes what this thing does?

normen662 · 2025-10-20T08:46:00Z

...c/main/java/com/apple/foundationdb/record/query/plan/cascades/UserDefinedScalarFunction.java

@@ -100,18 +100,18 @@ public static MacroFunction fromProto(@Nonnull final PlanSerializationContext se
     * Deserializer.
     */
    @AutoService(PlanDeserializer.class)
-    public static class Deserializer implements PlanDeserializer<PMacroFunctionValue, MacroFunction> {
+    public static class Deserializer implements PlanDeserializer<PUserDefinedScalarFunctionValue, UserDefinedScalarFunction> {


Can we please refrain from using plan serializer logic here. Can you refactor this logic to express the dynamic dispatch in a common way that plan serialization and this logic + @hatyo 's view all use the same underlying logic?

I don't quite understand here, isn't RawSqlFunction using the same logic? let's sync offline.

normen662 · 2025-10-20T08:48:19Z

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/RecordMetaDataBuilder.java

        }
+        PlanSerializationContext serializationContext = null;
        for (RecordMetaDataProto.PUserDefinedFunction function: metaDataProto.getUserDefinedFunctionsList()) {
+            if (serializationContext == null) {


This is not needed in a refactored world. Strictly speaking, the plan serialization context is necessary state such that repeated sub plans/values/types do not get serialized repeatedly. It a little bit like dictionary encoding. I think this is not needed here.

normen662 · 2025-10-20T08:57:02Z

fdb-record-layer-core/src/main/proto/record_query_plan.proto

    PNumericAggregationValue.PBitmapConstructAgg numeric_aggregation_value_bitmap_construct_agg = 45;
    PQuantifiedRecordValue quantified_record_value = 46;
-    PMacroFunctionValue macro_function_value = 47;
+    PUserDefinedScalarFunctionValue macro_function_value = 47;


PUserDefinedScalarFunctionValues counterpart is not a Value so this should be defined here. Also the name should be PUserDefinedScalarFunction because that's its counterpart. This file should not be touched by this PR at all I think, it should all go into metadata.

normen662 · 2025-10-20T08:57:58Z

fdb-record-layer-core/src/main/proto/record_metadata.proto

 message PUserDefinedFunction {
  oneof specific_function {
-    com.apple.foundationdb.record.planprotos.PMacroFunctionValue macro_function = 1;
+    com.apple.foundationdb.record.planprotos.PUserDefinedScalarFunctionValue user_defined_scalar_function = 1;


Please move that message definition to this file, rename it to PUserDefinedScalarFunction, see below.

normen662 · 2025-10-20T08:58:29Z

...st/java/com/apple/foundationdb/record/query/plan/cascades/UserDefinedScalarFunctionTest.java

@@ -0,0 +1,174 @@
+/*
+ * MacroFunctionTest.java


Suggested change

* MacroFunctionTest.java

* UserDefinedScalaFunctionTest.java

normen662 · 2025-10-29T10:11:47Z

...rc/main/java/com/apple/foundationdb/record/query/plan/cascades/UserDefinedMacroFunction.java


 /**
- * MacroFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.
+ * UserDefinedScalarFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.


Suggested change

* UserDefinedScalarFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.

* UserDefinedMacroFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.

github-actions · 2025-10-29T15:20:31Z

📊 Metrics Diff Analysis Report

Summary

New queries: 13
Dropped queries: 0
Plan changed + metrics changed: 0
Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

New queries: Queries added in this PR
Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
Plan changed + metrics changed: The query plan has changed along with planner metrics.
Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

yaml-tests/src/test/resources/user-defined-macro-function-tests.metrics.yaml: 13

This comment was marked as outdated.

Sign in to view

pengpeng-lu requested a review from hatyo January 16, 2025 16:44

normen662 requested changes Feb 3, 2025

View reviewed changes

normen662 requested a review from alecgrieser February 3, 2025 09:22

This comment was marked as outdated.

Sign in to view

pengpeng-lu added the Draft label Feb 20, 2025

pengpeng-lu added enhancement New feature or request and removed Draft labels Mar 21, 2025

pengpeng-lu marked this pull request as ready for review March 31, 2025 19:36

working

d0e0f94

pengpeng-lu commented Aug 4, 2025

View reviewed changes

pengpeng-lu force-pushed the udo branch from 684d5fc to d0e0f94 Compare August 4, 2025 23:33

pengpeng-lu added 7 commits August 4, 2025 22:46

refactor RLInvokedRoutine step 1

7c0bdae

refactor RLInvokedRoutine done

1e3f7b4

refactor parser

2ecb9c3

merge main

9fd1f1d

style

9cfdf52

Merge branch 'main' into udo

6cf351a

fix test

4b895f7

pengpeng-lu commented Aug 7, 2025

View reviewed changes

fdb-relational-core/src/main/antlr/RelationalParser.g4 Show resolved Hide resolved

pengpeng-lu added 2 commits August 19, 2025 11:51

style

c86f824

Merge branch 'main' into udo

08aa8e6

hatyo requested changes Aug 21, 2025

View reviewed changes

pengpeng-lu added 4 commits August 21, 2025 22:52

parser change

d18d804

InvokedRoutine

750e9be

lookupnestedField

5a82038

add temp scalar function tests in AstNormalizerTest

7514f03

pengpeng-lu added 6 commits October 2, 2025 15:21

more tests

31d3c8e

merge main

f1b6260

teamscale gaps

1f497ae

Merge branch 'main' into udo

930579b

style

eda5a37

merge main

2b0b77c

hatyo requested changes Oct 13, 2025

View reviewed changes

pengpeng-lu added 2 commits October 13, 2025 14:11

Merge branch 'main' into udo

1740682

add signature to TypedVisitor

04960e2

pengpeng-lu requested a review from hatyo October 13, 2025 22:28

hatyo approved these changes Oct 14, 2025

View reviewed changes

pengpeng-lu requested review from normen662 and removed request for alecgrieser October 14, 2025 19:15

normen662 requested changes Oct 20, 2025

View reviewed changes

pengpeng-lu added 2 commits October 21, 2025 15:04

rename

abbae80

Merge branch 'main' into udo

707eaa6

pengpeng-lu force-pushed the udo branch from 7c4b448 to 707eaa6 Compare October 21, 2025 22:07

pengpeng-lu added 2 commits October 24, 2025 12:53

serializationn modify

384c97d

style

b90f975

pengpeng-lu requested a review from normen662 October 26, 2025 18:20

pengpeng-lu added 2 commits October 27, 2025 16:29

Merge branch 'main' into udo

45f139f

add unit test Teamscale need

706f2b5

pengpeng-lu force-pushed the udo branch from 9663346 to 706f2b5 Compare October 27, 2025 23:46

pengpeng-lu added 2 commits October 28, 2025 14:00

merge main

59eeca0

fix test

404d4f4

normen662 requested changes Oct 29, 2025

View reviewed changes

typo

cc3c49a

normen662 approved these changes Oct 29, 2025

View reviewed changes

pengpeng-lu merged commit f47d9a8 into FoundationDB:main Oct 29, 2025
9 checks passed

	@Nonnull
	@Override
	public Identifier visitUid(@Nonnull RelationalParser.UidContext uidContext) {
	if (uidContext.simpleId() != null) {
	return visitSimpleId(uidContext.simpleId());
	} else {
	return Identifier.of(getDelegate().normalizeString(uidContext.getText()));
	}
	}

                       }
                   }
+                  @Test

	* UserDefinedScalarFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.
	* UserDefinedMacroFunction that expands a body (referring to parameters) into a {@link Value} (through encapsulation) call site.

Support UDF in plan generator #3040

Support UDF in plan generator #3040

Uh oh!

Conversation

pengpeng-lu commented Jan 14, 2025 • edited by hatyo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

normen662 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hatyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 29, 2025

pengpeng-lu commented Jan 14, 2025 •

edited by hatyo

Loading