[Feature] support named arguments for table functions #36596

fzhedu · 2023-12-07T03:45:07Z

support named arguments for table functions, keep the same behavior with Trino:

all parameters should be named arguments, without mixing positional arguments;
can mix default parameters
can reorder named arguments

MySQL > select * from TABLE(generate_series(start=>2, end=>5, step=>2));
+-----------------+
| generate_series |
+-----------------+
|               2 |
|               4 |
+-----------------+

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

doc

fzhedu · 2023-12-08T13:08:52Z

fe/fe-core/src/main/java/com/starrocks/catalog/Function.java

                    continue;
                }
-                if (!Type.isImplicitlyCastable(other.argTypes[i], getVarArgsType(), true)) {
+                if (!Type.isImplicitlyCastable(other.argTypes[i], this.argTypes[i], true)) {


wrong flags

imay

Please tell people why and what you are doing in this CL.

imay · 2023-12-08T23:07:39Z

test/sql/test_function/R/test_named_argments

+select * from TABLE(generate_series(start=>2, stop=>5));
+-- result:
+E: (1064, "Getting analyzing error. Detail message: Unknown table function 'generate_series(start=>2,stop=>5)', the function doesn't support named arguments or has invalid arguments.")


step is an optional argument, why it is not supported?

the second should be end, not stop, invalid arguments.

imay · 2023-12-08T23:08:12Z

test/sql/test_function/T/test_named_argments

@@ -0,0 +1,12 @@
+-- name: test named arguments for table functions
+
+select * from TABLE(generate_series(start=>2, end=>5));


You need to add tests to cover normal cases.
I see all cases are fail cases.

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/ExpressionAnalyzer.java

docs/en/sql-reference/sql-functions/table-functions/generate_series.md

Seaven · 2023-12-13T02:36:02Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4

@@ -2158,6 +2158,7 @@ expression
    | NOT expression                                                                      #logicalNot
    | left=expression operator=(AND|LOGICAL_AND) right=expression                         #logicalBinary
    | left=expression operator=(OR|LOGICAL_OR) right=expression                           #logicalBinary
+    | identifier '=>' expression                                                          #namedArgument


add it in functioncall is better?

or new a special expression ArgumentExpression to resolve all function arguments? adding here will contaminate all expressions

is param1 => param2 => value_expr valid?

may use a new rule like expression ('=>' expression) ? to decribe the item in table fucntion arguments?

introduce namedArgumentList to narrow scop needed.
param1 => param2 => value_expr is invalid.

Seaven · 2023-12-13T02:43:48Z

fe/fe-core/src/main/java/com/starrocks/catalog/Function.java

+            boolean[] mask = new boolean[other.argTypes.length];
+            for (int j = startArgIndex; j < other.argTypes.length; ++j) {
+                boolean found = false;
+                for (int i = startArgIndex; i < this.argTypes.length; ++i) {
+                    if (!mask[i] && this.argNames[i].equals(other.argNames[j])) {
+                        if (!other.argTypes[j].matchesType(this.argTypes[i]) &&
+                                !Type.isImplicitlyCastable(other.argTypes[j], this.argTypes[i], true)) {
+                            return false;
+                        }
+                        found = true;
+                        mask[i] = true;
+                    }
+                }


I think maybe you need extract some method, like getArgumentByName ?

Seaven · 2023-12-13T02:49:03Z

fe/fe-core/src/main/java/com/starrocks/catalog/Function.java

+    @SerializedName(value = "argNames")
+    private String[] argNames;
+


BTW, why don't support a special class to resolve arguments? may be we need support all function in future? separate to handle the arguments and name isn't a sustainable practice

and if we need support postiation arguments and name arguments at sametime, define a unified abstraction is better?

Adding named argumetns seems change a few codes, seems no need add new class.

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

sonarqubecloud · 2023-12-14T16:02:26Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

22 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2023-12-15T03:13:10Z

[FE Incremental Coverage Report]

❌ fail : 145 / 183 (79.23%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/sql/ast/AstVisitor.java	0	1	00.00%	[1256]
🔵	com/starrocks/sql/analyzer/ExpressionAnalyzer.java	0	1	00.00%	[1957]
🔵	com/starrocks/analysis/NamedArgument.java	6	16	37.50%	[29, 30, 31, 32, 43, 44, 48, 53, 58, 63]
🔵	com/starrocks/catalog/Function.java	83	109	76.15%	[184, 543, 554, 555, 557, 558, 559, 560, 562, 563, 567, 568, 569, 571, 572, 576, 579, 580, 590, 728, 729, 730, 764, 765, 766, 767]
🔵	com/starrocks/catalog/FunctionSet.java	2	2	100.00%	[]
🔵	com/starrocks/analysis/FunctionParams.java	25	25	100.00%	[]
🔵	com/starrocks/analysis/Expr.java	5	5	100.00%	[]
🔵	com/starrocks/sql/analyzer/QueryAnalyzer.java	13	13	100.00%	[]
🔵	com/starrocks/catalog/TableFunction.java	11	11	100.00%	[]

github-actions · 2023-12-15T03:13:19Z

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com> Signed-off-by: 张敢 <zhanggan@deepexi.com>

fzhedu requested review from a team as code owners December 7, 2023 03:45

github-actions bot added behavior_changed META-REVIEW labels Dec 7, 2023

mergify bot assigned fzhedu Dec 7, 2023

fzhedu force-pushed the nameArg branch from bc2f736 to 92dd841 Compare December 7, 2023 03:57

fzhedu commented Dec 8, 2023

View reviewed changes

github-actions bot added the documentation Improvements or additions to documentation label Dec 8, 2023

fzhedu force-pushed the nameArg branch from 9fcccc2 to 27a04a0 Compare December 8, 2023 13:32

github-actions bot removed the behavior_changed label Dec 8, 2023

imay reviewed Dec 8, 2023

View reviewed changes

packy92 reviewed Dec 11, 2023

View reviewed changes

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/ExpressionAnalyzer.java Show resolved Hide resolved

docs/en/sql-reference/sql-functions/table-functions/generate_series.md Show resolved Hide resolved

fzhedu requested review from packy92 and imay December 11, 2023 09:57

Seaven reviewed Dec 13, 2023

View reviewed changes

fzhedu force-pushed the nameArg branch from 49c94c9 to c1a1a61 Compare December 14, 2023 03:43

packy92 previously approved these changes Dec 14, 2023

View reviewed changes

fzhedu dismissed packy92’s stale review via a56679b December 14, 2023 08:49

fzhedu added 9 commits December 14, 2023 22:09

[Feature] support named arguments for table functions

c84b966

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

update order

f3deb69

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

add check

3caa306

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

add tests

9ebfc43

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

update doc

fbc60ab

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

add more tests

46d6925

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

fix comments

9101546

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

updated

348d81d

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

update tests

312333e

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

fzhedu force-pushed the nameArg branch from a56679b to 312333e Compare December 14, 2023 14:09

update tests

02558f5

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com>

Seaven approved these changes Dec 15, 2023

View reviewed changes

HangyuanLiu approved these changes Dec 15, 2023

View reviewed changes

kangkaisen approved these changes Dec 15, 2023

View reviewed changes

kangkaisen merged commit 33578e5 into StarRocks:main Dec 15, 2023
44 of 45 checks passed

Zhangg7723 pushed a commit to Zhangg7723/starrocks that referenced this pull request Dec 26, 2023

[Feature] support named arguments for table functions (StarRocks#36596)

9535e49

Signed-off-by: Zhuhe Fang <fzhedu@gmail.com> Signed-off-by: 张敢 <zhanggan@deepexi.com>

banmoy mentioned this pull request Feb 13, 2025

[BugFix] Fix FunctionParams NPE #55885

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] support named arguments for table functions #36596

[Feature] support named arguments for table functions #36596

fzhedu commented Dec 7, 2023 •

edited

Loading

fzhedu Dec 8, 2023

imay left a comment

imay Dec 8, 2023

fzhedu Dec 11, 2023

imay Dec 8, 2023

fzhedu Dec 11, 2023

Seaven Dec 13, 2023

Seaven Dec 13, 2023

jaogoy Dec 13, 2023

packy92 Dec 13, 2023

fzhedu Dec 14, 2023

Seaven Dec 13, 2023

fzhedu Dec 14, 2023

Seaven Dec 13, 2023

Seaven Dec 13, 2023

fzhedu Dec 14, 2023

sonarqubecloud bot commented Dec 14, 2023

github-actions bot commented Dec 15, 2023

github-actions bot commented Dec 15, 2023

		@@ -0,0 +1,12 @@
		-- name: test named arguments for table functions

		select * from TABLE(generate_series(start=>2, end=>5));

		@SerializedName(value = "argNames")
		private String[] argNames;

[Feature] support named arguments for table functions #36596

[Feature] support named arguments for table functions #36596

Conversation

fzhedu commented Dec 7, 2023 • edited Loading

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

Choose a reason for hiding this comment

imay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 14, 2023

Quality Gate passed

github-actions bot commented Dec 15, 2023

[FE Incremental Coverage Report]

file detail

github-actions bot commented Dec 15, 2023

[BE Incremental Coverage Report]

fzhedu commented Dec 7, 2023 •

edited

Loading