[DRAFT] Initializes PartiQL-system interfaces and document. #1480

RCHowell · 2024-06-03T19:43:46Z

Description

https://github.com/partiql/partiql-lang-kotlin/blob/v1-path/partiql-planner/README.adoc

This PR initializes the interfaces for the 1.0 planner interfaces which model the PartiQL-system. This review is meant to collect feedback on the high-level public interfaces and documentation before updating internals.

Details can be found in the README.adoc.

License Information

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

johnedquinn · 2024-06-03T20:35:54Z

partiql-planner/README.adoc

Very glad to see this document. Kudos for taking the initiative.

johnedquinn · 2024-06-03T20:41:44Z

partiql-planner/src/main/kotlin/org/partiql/planner/metadata/Function.kt

+     * @property isNullCall
+     */
+    public data class Properties(
+        @JvmField public val isNullCall: Boolean = false,


No isMissingCall? Are you expecting to add it later?

No I don't believe it's necessary for function calls, only some very special operators (which will come later).

johnedquinn · 2024-06-03T20:51:15Z

partiql-planner/README.adoc

+
+Let _N_ be a name with steps _n~1~ . n~2~ ... n~i~_ and let _S_ be the session scope with name _s~1~_ ... _s~j~_.
+
+. Let _T_ be the fully-qualified table name.


The guide might be a bit confusing due to the conflation of SQL terms and PartiQL terms. From above: A name is composed of _n_ steps and can be qualified (_n > 1_) or unqualified (_n = 1_). It seems like the use of "qualified" only indicates whether a name has more than 1 step (regardless of whether the name is absolute or relative).

Then, A _relative_ name begins with a step whereas an _absolute_ name begins with a leading period..

I almost feel as if we should begin to deprecate the use of "qualified". It could just be "absolute path/name". Essentially, does the use of SQL's "qualified" give any additional meaning now with the concept of relative & absolute?

On a similar note, depending on your thoughts on this, it may be a good idea to specify what the root of the system is. Is it a scope? For first launch, it may be useful to say that it is not a scope (so that tables/views/etc) cannot be created outside of a catalog.

Yes, you can have a relative-qualified path.

x.y.z -- relative-qualified .a.b.c -- absolute-qualified

I don't think "qualified" adds any value here though, and should just be "relative name" and "absolute name".

johnedquinn · 2024-06-03T20:52:52Z

partiql-planner/src/main/kotlin/org/partiql/planner/metadata/Table.kt

+    public fun getKind(): Kind = Kind.TABLE
+
+    /**
+     * The table's shape; in SQL this is called 'table schema'.


Suggested change

* The table's shape; in SQL this is called 'table schema'.

* The table's shape; in SQL this is called 'table descriptor'.

johnedquinn · 2024-06-03T20:53:52Z

partiql-planner/src/main/kotlin/org/partiql/planner/metadata/Name.kt

+ *
+ * @property steps
+ */
+public data class Name(public val steps: List<String>) : Iterable<String> {


Is this supposed to represent an Absolute Name or Relative Name?

Neither, it's just a name.

johnedquinn · 2024-06-03T21:00:11Z

partiql-planner/src/main/kotlin/org/partiql/planner/metadata/Scope.kt

+     * @param name  The case-sensitive [Table] name.
+     * @return      The [Table] or null if not found.
+     */
+    public fun getTable(name: String): Table? = null


For this method (as well as the ones below), I'm wondering how we can determine the case-sensitive names for third-party consumers (that may not consistently upper-or-lower-case their table names upon storage).

We don't determine the case-sensitive names; I think you have consumer-implementer backwards. We case-normalize to lowercase and it's up to the implementer how to handle.

In other words, all usage of the Scope comes from the planner; so the planner is only "consumer" of Scope — implementers are responsible for handling their own mapping from PartiQL's case-normalized

johnedquinn · 2024-06-03T21:19:15Z

partiql-planner/README.adoc

+Let _N_ be a routine name with steps _n~1~ . n~2~ ... n~i~_ and let _S_ be the session scope with name _s~1~_ ... _s~j~_.
+
+. If n = 1 then the session path is searched left-to-right for _n~1~_.
+. If n > 1, then let _R_ be the fully-qualified routine name.
+.. If _N_ is a relative name, then _R = s~1~ . s~2~ ... s~j~ . n~1~ . n~2~. ... n~i~_ .
+.. If _N_ is an absolute name, then _R = n~1~ . n~2~ ... n~i~_ .
+. Lookup the fully-qualified name _R_ and fail compilation if not found.


Regarding "fully-qualified" vs "absolute", I think the following suggestion is a bit more clear.

Suggested change

Let _N_ be a routine name with steps _n~1~ . n~2~ ... n~i~_ and let _S_ be the session scope with name _s~1~_ ... _s~j~_.

. If n = 1 then the session path is searched left-to-right for _n~1~_.

. If n > 1, then let _R_ be the fully-qualified routine name.

.. If _N_ is a relative name, then _R = s~1~ . s~2~ ... s~j~ . n~1~ . n~2~. ... n~i~_ .

.. If _N_ is an absolute name, then _R = n~1~ . n~2~ ... n~i~_ .

. Lookup the fully-qualified name _R_ and fail compilation if not found.

Let _N_ be the routine invocation name (absolute or relative) with steps _n1_, _n2_, …, _ni_. Let _S_ be the session scope with name _.s1.s2. … sj_. Let _R_ by the absolute name of the invoked routine.

1. If _N_ is an absolute name, then _R = . n1 . n2 … ni_ . Return _R_.

2. If n = 1 then the function path is searched left-to-right for _n1_.

3. If n > 1, then _R = . s1 . s2 … sj . n1 . n2. … ni_ .

4. Lookup the absolute name _R_. Return function(s) if found. Fail compilation if not found.

Maybe the "function path" should be SQL Path or invocation path. Up to you.

Edit, it's meant to be "session path" from the original text.

github-actions · 2024-06-14T23:50:10Z

Conformance comparison report-Cross Engine

	Base (legacy)	eval	+/-
% Passing	92.62%	15.68%	-76.94%
✅ Passing	5422	918	-4504
❌ Failing	432	4936	4504
🔶 Ignored	0	0	0
Total Tests	5854	5854	0
Number passing in both: 881

Number failing in both: 395

Number passing in legacy engine but fail in eval engine: 4541

Number failing in legacy engine but pass in eval engine: 37
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
37 test(s) were failing in legacy but now pass in eval. Before merging, confirm they are intended to pass.
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

Conformance comparison report-Cross Commit-LEGACY

	Base (`4dd0972`)	`a0103d6`	+/-
% Passing	92.60%	92.62%	0.02%
✅ Passing	5421	5422	1
❌ Failing	433	432	-1
🔶 Ignored	0	0	0
Total Tests	5854	5854	0
Number passing in both: 5421

Number failing in both: 432

Number passing in Base (4dd0972) but now fail: 0

Number failing in Base (4dd0972) but now pass: 1
The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see

MYSQL_SELECT_29, compileOption: LEGACY

Conformance comparison report-Cross Commit-EVAL

	Base (`4dd0972`)	`a0103d6`	+/-
% Passing	87.05%	15.68%	-71.37%
✅ Passing	5097	918	-4179
❌ Failing	758	4936	4178
🔶 Ignored	0	0	0
Total Tests	5855	5854	-1
Number passing in both: 918

Number failing in both: 758

Number passing in Base (4dd0972) but now fail: 4179

Number failing in Base (4dd0972) but now pass: 0
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

RCHowell requested a review from johnedquinn June 3, 2024 19:44

johnedquinn reviewed Jun 3, 2024

View reviewed changes

RCHowell force-pushed the v1-path branch from 9c19700 to 870f7c6 Compare June 14, 2024 23:42

RCHowell force-pushed the v1-path branch from 60b77ff to 6fcde57 Compare June 20, 2024 17:49

RCHowell added 5 commits June 20, 2024 14:45

Adds v1-planner interfaces

f41e416

Start moving builtins out of SPI

80da43d

Backup function moves

dff2a15

Backup

bcc8ade

Rebase and fix assemble

956a80d

RCHowell force-pushed the v1-path branch from 6fcde57 to 956a80d Compare June 24, 2024 15:28

RCHowell closed this Jun 24, 2024

RCHowell deleted the v1-path branch July 18, 2024 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Initializes PartiQL-system interfaces and document. #1480

[DRAFT] Initializes PartiQL-system interfaces and document. #1480

RCHowell commented Jun 3, 2024 •

edited

Loading

johnedquinn Jun 3, 2024

johnedquinn Jun 3, 2024

RCHowell Jun 4, 2024 •

edited

Loading

johnedquinn Jun 3, 2024

RCHowell Jun 4, 2024

johnedquinn Jun 3, 2024

johnedquinn Jun 3, 2024

RCHowell Jun 4, 2024

johnedquinn Jun 3, 2024

RCHowell Jun 4, 2024

johnedquinn Jun 3, 2024 •

edited

Loading

RCHowell Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024 •

edited

Loading


		Let _N_ be a name with steps _n~1~ . n~2~ ... n~i~_ and let _S_ be the session scope with name _s~1~_ ... _s~j~_.

		. Let _T_ be the fully-qualified table name.

	* The table's shape; in SQL this is called 'table schema'.
	* The table's shape; in SQL this is called 'table descriptor'.

[DRAFT] Initializes PartiQL-system interfaces and document. #1480

[DRAFT] Initializes PartiQL-system interfaces and document. #1480

Conversation

RCHowell commented Jun 3, 2024 • edited Loading

Description

License Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCHowell Jun 4, 2024 • edited Loading

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

johnedquinn Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

RCHowell Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2024 • edited Loading

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-LEGACY

Conformance comparison report-Cross Commit-EVAL

RCHowell commented Jun 3, 2024 •

edited

Loading

RCHowell Jun 4, 2024 •

edited

Loading

johnedquinn Jun 3, 2024 •

edited

Loading

RCHowell Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024 •

edited

Loading