Add stored procedure calls with unnamed args #345

alancai98 · 2020-12-17T00:46:49Z

Adds the ability to create custom stored procedures and call them with the EXEC clause, following the description given in this issue.

Also provides a simple custom stored procedure example.

Previously approved PRs part of this PR:

parsing + PIG definition Stored Procedures - call with unnamed args (parsing + PIG definition) #333
evaluation + example Stored Procedures - call with unnamed args (evaluation) #335

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…#333)

codecov-io · 2020-12-24T22:33:38Z

Codecov Report

Merging #345 (d218a50) into master (a762ee1) will decrease coverage by 0.32%.
The diff coverage is 61.03%.

@@             Coverage Diff              @@
##             master     #345      +/-   ##
============================================
- Coverage     82.37%   82.04%   -0.33%     
- Complexity     1233     1244      +11     
============================================
  Files           157      159       +2     
  Lines          9366     9514     +148     
  Branches       1526     1546      +20     
============================================
+ Hits           7715     7806      +91     
- Misses         1192     1244      +52     
- Partials        459      464       +5

Flag	Coverage Δ	Complexity Δ
CLI	`18.11% <ø> (ø)`	`0.00 <ø> (ø)`
EXAMPLES	`75.80% <74.46%> (-0.22%)`	`0.00 <2.00> (ø)`
LANG	`84.65% <55.14%> (-0.37%)`	`0.00 <8.00> (ø)`
PTS	`100.00% <ø> (ø)`	`0.00 <ø> (ø)`
TEST_SCRIPT	`79.68% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...mples/src/kotlin/org/partiql/examples/util/Main.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/ast/AstSerialization.kt	`87.84% <0.00%> (-0.47%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/ast/passes/AstWalker.kt	`83.20% <0.00%> (-0.65%)`	`32.00 <0.00> (ø)`
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt	`78.33% <0.00%> (-2.88%)`	`151.00 <0.00> (ø)`
...g/eval/builtins/storedprocedure/StoredProcedure.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt	`92.59% <55.55%> (-0.63%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/CompilerPipeline.kt	`80.95% <62.50%> (-2.84%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/ast/ast.kt	`90.25% <75.00%> (-0.32%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/syntax/SqlParser.kt	`80.43% <75.00%> (-0.03%)`	`237.00 <7.00> (+7.00)`	⬇️
...in/org/partiql/examples/CustomProceduresExample.kt	`76.08% <76.08%> (ø)`	`2.00 <2.00> (?)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a762ee1...d218a50. Read the comment docs.

Zhouenze · 2021-01-07T01:35:01Z

lang/src/org/partiql/lang/ast/ExprNodeToStatement.kt

+
+    return PartiqlAst.build {
+        when (node) {
+            is Exec -> exec(node.procedureName.name, node.args.map { it.toAstExpr() }, metas)


procedureName.meta is not kept in PartiqlAst.Statement? Not to say it's important but just want to confirm.

That particular meta may not be too important, but we should be copying metas wherever possible. I will add that in the next commit.

I noticed there are quite a few other metas that aren't copied when converting ExprNode<-> PartiqlAst, so I created an issue (#353) to track.

Zhouenze · 2021-01-07T01:55:21Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

@@ -2214,9 +2242,23 @@ class SqlParser(private val ion: IonSystem) : Parser {
        return ParseNode(ARG_LIST, null, items, rem)
    }

+    /**


There are other tokens that should only exist at top level right? Why is EXEC different and requires a separate check?

The other tokens that should only exist at the top level (i.e. DML and DDL ops) weren't properly checked in the parser (created an issue #354 to add these checks in the future). I will rename the checkForUnexpectedExec() to something more general for all tokens that should only exist in the top level.

Zhouenze · 2021-01-07T01:59:08Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

+            Property.TOKEN_VALUE to ion.newSymbol("exec")))
+
+    @Test
+    fun execAtUnexpectedLocationInExpression() = checkInputThrowingParserException(


What will happen for SELECT * FROM EXEC or SELECT * FROM exec?

Currently for both of those queries, the PARSE_EXEC_AT_UNEXPECTED_LOCATION will be thrown. Perhaps I can make it more general for other tokens that should only be at the top-level.

Zhouenze · 2021-01-07T18:10:19Z

lang/src/org/partiql/lang/eval/builtins/storedprocedure/StoredProcedure.kt

+
+    /**
+     * Invokes the stored procedure. Proper arity is checked by the [EvaluatingCompiler], but argument type checking
+     * is left to the implementation.


Not related to this change but wonder if there could be some utility for user to easily build their type check. Would be useful in many places, not limited to stored procedure. BTW I heard ExprValue is going to be deprecated? Maybe it's better to consider utility after that.

We have some future plans to add a utility to help type-check stored procedures and user-defined functions.

ExprValue will likely be deprecated sometime in the future, so the utility will probably be implemented with its replacement.

Zhouenze · 2021-01-07T18:20:15Z

lang/src/org/partiql/lang/errors/ErrorCode.kt

@@ -432,6 +445,16 @@ enum class ErrorCode(private val category: ErrorCategory,
            "got: ${errorContext?.get(Property.ACTUAL_ARGUMENT_TYPES) ?: UNKNOWN}"
    },

+    EVALUATOR_INCORRECT_TYPE_OF_ARGUMENTS_TO_PROCEDURE_CALL(


According to comment, type check is done in procedure implementation, so usage of this error code depends on user's best intention. As mentioned in the other comment, this could be improved by taking over type check or providing utility to help user defining their own type check.

As mentioned in the other reply, we do have plans to add a type-checking utility for sprocs and udfs, so once that's added, throwing of this error should be handled by that utility.

Zhouenze · 2021-01-07T18:46:04Z

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

+            internal = false)
+
+        // Check arity
+        if (args.size !in procedure.signature.arity) {


Just found something weird regarding IntRange: IntRange(2,1) is valid, no exception, but internally it assumes start <= end (link). 1 in IntRange(1,2)=true but 1 in IntRange(2,1)=false. Not to say we should do anything about it but maybe good to know.

Hmm interesting. I don't think that internally IntRange assumes start <= end. The Kotlin 1.3 jar even defines an EMPTY range as IntRange(1, 0). It seems in (i.e. contains) only returns true if and only if start <= value && value <= last, so the behavior makes some sense.

This leads me to think that nothing should be done unless we decide not to use IntRange.

alancai98 added 2 commits December 3, 2020 15:20

Stored Procedures - call with unnamed args (parsing + PIG definition) (…

922d4a3

…#333)

Stored Procedures - call with unnamed args (evaluation) (#335)

6c27840

alancai98 requested review from therapon, dlurton and abhikuhikar December 17, 2020 00:46

alancai98 self-assigned this Dec 17, 2020

Update examples/Readme.md

d218a50

Zhouenze reviewed Jan 7, 2021

View reviewed changes

Zhouenze previously approved these changes Jan 7, 2021

View reviewed changes

alancai98 mentioned this pull request Jan 7, 2021

Add general parser checks for top-level only tokens (i.e. DML and DDL ops) #354

Closed

Pass sproc name metas, modify top-level token checks

c6aea9d

alancai98 dismissed Zhouenze’s stale review via c6aea9d January 7, 2021 21:03

Zhouenze approved these changes Jan 7, 2021

View reviewed changes

dlurton approved these changes Jan 7, 2021

View reviewed changes

alancai98 merged commit 260d5ce into master Jan 11, 2021

alancai98 deleted the stored-procedure branch January 11, 2021 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stored procedure calls with unnamed args #345

Add stored procedure calls with unnamed args #345

alancai98 commented Dec 17, 2020 •

edited

Loading

codecov-io commented Dec 24, 2020 •

edited

Loading

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Zhouenze Jan 7, 2021

alancai98 Jan 7, 2021

Add stored procedure calls with unnamed args #345

Add stored procedure calls with unnamed args #345

Conversation

alancai98 commented Dec 17, 2020 • edited Loading

codecov-io commented Dec 24, 2020 • edited Loading

Codecov Report

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

alancai98 commented Dec 17, 2020 •

edited

Loading

codecov-io commented Dec 24, 2020 •

edited

Loading