Initializes PartiQL's error reporting mechanism #1615

johnedquinn · 2024-10-08T17:22:59Z

Relevant Issues

Closes Add uniform error reporting (codes) across parser, planner, and evaluator #1597

Description

Creates V1's error reporting mechanism, highlighted by the introduction of PError, PErrorListener, and PErrorListenerException.
- Other notable additions: PEnum, Severity, and Classification.
Now, ALL errors/warnings will be reported via the registered error listener (for each component).
I've added an implementation of this in the CLI.
I've also added a usage guide (that may be updated prior to the release of V1). This is just a start.
I've completely removed the legacy Error and Problem APIs.

Follow-ups

Need to remove PartiQLSyntaxException -- which was made internal by this PR. But we no longer need to convert to a syntax exception and then to an error code.

License Information

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

docs/wiki/v1/ErrorHandling.md

RCHowell · 2024-10-17T15:30:53Z

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngine.kt

@@ -24,7 +24,7 @@ import org.partiql.spi.catalog.Session
 */
 public interface PartiQLEngine {

-    public fun prepare(plan: Plan, mode: Mode, session: Session): PartiQLStatement
+    public fun prepare(plan: Plan, session: Session, config: CompilerConfig): PartiQLStatement


I suggest changing the "Config" to something more like Context which may or may not be part of the session, but perhaps these are slightly different?

The Config reads like what should be part of a PartiQLEngine/PartiQLParser/PartiQLPlanner builders. Why not have PartiQLParser.builder().addListener(...) because the "config" is what the constructor arguments are for.

I am curious the use-case of having a changing config for each invocation, and where a session context wouldn't achieve this.

Having the error listener as part of a context/config allows for end-users to create singleton planners (if they want to). They're given the freedom to always pass the same object, or to pass in new error listeners.

That being said, I'm up for adding it to the session context -- but I wasn't entirely sure the right place to put it.

RCHowell · 2024-10-17T15:38:48Z

partiql-parser/src/main/java/org/partiql/parser/ParserConfig.java

+/**
+ * Represents the configuration for the {@link PartiQLParser}.
+ *
+ * @see PartiQLParser
+ * @see ParserConfigBuilder
+ */
+public interface ParserConfig {
+    /**
+     * @return The error listener to be used by the compiler.
+     */
+    @NotNull
+    PErrorListener getErrorListener();
+}


What are some reasons not to add listeners via constructors (or even mutable class methods) like ANTLR has. These configs add a several more classes to juggle and thread through all calls, which seems like additional complexity but I may be missing how these are more useful than just adding listeners via the builders.

See #1615 (comment).

partiql-parser/src/main/kotlin/org/partiql/parser/PartiQLParser.kt

partiql-spi/src/main/java/org/partiql/spi/errors/PError.java

partiql-planner/src/main/kotlin/org/partiql/planner/internal/problems/AlwaysMissing.kt

RCHowell · 2024-10-17T18:15:45Z

partiql-spi/src/main/java/org/partiql/spi/errors/Classification.java

+    @NotNull
+    public static Classification COMPILATION() {
+        return new Classification(COMPILATION);
+    }
+
+    /**
+     * TODO
+     */
+    public static final int EXECUTION = 4;


What are some examples of compilation vs execution errors?

In a recent message we were talking about how lazy datums hide execution problems. I'm not sure I understand how we might get an execution PError rather than something akin to a "runtime exception" when materializing a lazy datum.

Perhaps some driver in partiql-lang would take a listener an it might emit execution errors? I somewhat think partiql-eval should have an evaluator driver that can emit them. By driver, I just mean something that materializes Datums. So a customer may choose to use the lazy datum, or just invoke an eval() which actually performs the work and accepts a listener. Does that make sense?

What are some examples of compilation vs execution errors?

Compilation errors might happen if the plan is referencing a custom physical operator that doesn't exist, or if a feature is not supported for execution (yet). It describes taking the process of taking the input plan and outputting an executable. There may be many types of errors at this stage.

Execution errors may be emitted during actual execution, such as data exceptions. I actually discuss this briefly in the usage guide, but the idea is that all components will standardize the fact the the only JVM exception that these components will see is the PErrorListenerException. Though, for exceptions that arise during the execution of the prepared statement, I detailed an EvaluationException that essentially wraps a PError.

partiql-spi/src/main/java/org/partiql/spi/errors/Classification.java

Adds multiple error reporting options to the CLI Adds usage guide

Renames Error to Problem Internalizes the construction of problems Creates a PEnum

Adds Javadocs to PErrorKind and Severity

docs/wiki/v1/ErrorHandling.md

partiql-eval/src/main/java/org/partiql/eval/CompilerContext.java

partiql-planner/src/main/kotlin/org/partiql/planner/PartiQLPlannerPass.kt

Creates a shared Context in SPI

Updates usage guide

RCHowell · 2024-10-24T20:44:10Z

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngine.kt

+    public fun prepare(plan: Plan, session: Session, ctx: Context): PartiQLStatement {
+        return prepare(plan, session, ctx, Mode.STRICT)
+    }
+
+    public fun prepare(plan: Plan, session: Session, mode: Mode): PartiQLStatement {
+        return prepare(plan, session, Context.standard(), mode)
    }


no default mode and the ctx with/without are the overloads so something like

Suggested change

public fun prepare(plan: Plan, session: Session, ctx: Context): PartiQLStatement {

return prepare(plan, session, ctx, Mode.STRICT)

}

public fun prepare(plan: Plan, session: Session, mode: Mode): PartiQLStatement {

return prepare(plan, session, Context.standard(), mode)

}

public fun prepare(plan: Plan, mode: Mode): PartiQLStatement {

return prepare(plan, mode, Context.NONE)

}

public fun prepare(plan: Plan, mode: Mode, ctx: Context): PartiQLStatement

I can follow up since this will go on the java PartiQLCompiler interface so no more kt engine thing.

RCHowell · 2024-10-24T20:52:30Z

partiql-spi/src/main/java/org/partiql/spi/Context.java

+    @NotNull
+    default PErrorListener getErrorListener() {
+        return PErrorListener.abortOnError();
+    }


I'm not sure we want to instantiate a new error listener every time, this is somewhat like the argument changing everytime. We can have a TODO to make AbortOnError a static variable

johnedquinn force-pushed the v1-error-handling branch 5 times, most recently from 196565b to c51d9c7 Compare October 15, 2024 22:19

johnedquinn changed the title ~~[DRAFT] Adds V1 Error Handling~~ Initializes PartiQL's error reporting mechanism Oct 15, 2024

johnedquinn marked this pull request as ready for review October 15, 2024 22:41

johnedquinn requested a review from RCHowell October 15, 2024 22:41

RCHowell requested changes Oct 17, 2024

View reviewed changes

partiql-spi/src/main/java/org/partiql/spi/errors/PError.java Outdated Show resolved Hide resolved

partiql-planner/src/main/kotlin/org/partiql/planner/internal/problems/AlwaysMissing.kt Outdated Show resolved Hide resolved

RCHowell reviewed Oct 17, 2024

View reviewed changes

partiql-spi/src/main/java/org/partiql/spi/errors/Classification.java Outdated Show resolved Hide resolved

johnedquinn added 8 commits October 22, 2024 13:47

Initializes error reporting mechanism

bd061b4

Adds multiple error reporting options to the CLI Adds usage guide

Adds a Classification and Severity

540cf45

Renames Error to Problem Internalizes the construction of problems Creates a PEnum

Renames Classification to PErrorKind

f597bb7

Adds Javadocs to PErrorKind and Severity

Makes PError a final class

0e83d8b

Removes ProblemGenerator completely

03495fa

Removes legacy errors/problems

bfe1956

Updates error reporting usage guide

b0769a2

Renames context objects and uses Lombok for builders

a993ec5

johnedquinn force-pushed the v1-error-handling branch from cb248c2 to a993ec5 Compare October 22, 2024 21:41

johnedquinn requested a review from RCHowell October 22, 2024 21:49

RCHowell requested changes Oct 23, 2024

View reviewed changes

johnedquinn added 2 commits October 24, 2024 11:52

Removes component-specific contexts

36acf27

Creates a shared Context in SPI

Updates the PartiQLPlannerPass

be2d164

Updates usage guide

RCHowell approved these changes Oct 24, 2024

View reviewed changes

johnedquinn added 2 commits October 24, 2024 15:02

Merge remote-tracking branch 'upstream/v1' into v1-error-reporting-impl

5446fcd

Adds TODO for the static methods to construct listeners and contexts

7c389a1

RCHowell approved these changes Oct 24, 2024

View reviewed changes

johnedquinn merged commit 01979fb into partiql:v1 Oct 24, 2024
7 checks passed

johnedquinn deleted the v1-error-handling branch October 24, 2024 22:41

alancai98 mentioned this pull request Oct 25, 2024

[v1] Migrate parser to new AST #1626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initializes PartiQL's error reporting mechanism #1615

Initializes PartiQL's error reporting mechanism #1615

johnedquinn commented Oct 8, 2024 •

edited

Loading

RCHowell Oct 17, 2024

johnedquinn Oct 17, 2024

RCHowell Oct 17, 2024

johnedquinn Oct 18, 2024

RCHowell Oct 17, 2024

RCHowell Oct 17, 2024

johnedquinn Oct 22, 2024

RCHowell Oct 24, 2024

RCHowell Oct 24, 2024

Initializes PartiQL's error reporting mechanism #1615

Initializes PartiQL's error reporting mechanism #1615

Conversation

johnedquinn commented Oct 8, 2024 • edited Loading

Relevant Issues

Description

Follow-ups

License Information

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

johnedquinn commented Oct 8, 2024 •

edited

Loading