Spark 3.5: Iceberg parser should passthrough unsupported procedure to delegate

[pan3793]

Spark allows users to configure multiple extensions and each extension is allowed to inject its own SQL parser, there is a chance that the user configures multiple extensions that support CALL syntax, ideally, each extension should only intercept its supported procedure and leave others to the delegate.

Currently, all Iceberg builtin procedures are under system namespace, which is guard by

iceberg/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/BaseCatalog.java

Line 51 in 81b3310

if (isSystemNamespace(namespace)) {

so I propose to add a simple rule to check the SQL before parsing

[pan3793]

cc @RussellSpitzer @Fokko @aokolnychyi could you please take a look?

[RussellSpitzer]

In 4.0 we won't need this correct? One of the things I don't like about this (but I don't think there is a fix in 3.5) is that Parse errors are very uninformative.

[huaxingao]

Spark already has error class in 3.5. Shall we do something like

    assertThatThrownBy(() -> parser.parsePlan("CALL cat.d.t()"))
        .isInstanceOf(ParseException.class)
        .satisfies(exception -> {
            ParseException parseException = (ParseException) exception;
            assertThat(parseException.getErrorClass()).isEqualTo("PARSE_SYNTAX_ERROR");
            assertThat(parseException.getMessageParameters().get("error")).isEqualTo("'CALL'");
         });

[pan3793]

In 4.0 we won't need this correct? One of the things I don't like about this (but I don't think there is a fix in 3.5) is that Parse errors are very uninformative.

@RussellSpitzer Yes, with SPARK-44167, Spark supports to parse the CALL statement, and a FAILED_TO_LOAD_ROUTINE error class will be thrown for this case.

@huaxingao thank you for information, will change soon.

[pan3793]

@huaxingao test assertions are updated as suggested, please take another look, thank you in advance

[RussellSpitzer]

why not add the "system" here? do we want access to the list of names without the "system" prefix?

[pan3793]

to make it consistent with the existing method - the parameter name does not contain the system. prefix

public static ProcedureBuilder newBuilder(String name)

[RussellSpitzer]

Spark allows users to configure multiple extensions and each extension is allowed to inject its own SQL parser, there is a chance that the user configures multiple extensions that support CALL syntax, ideally, each extension should only intercept its supported procedure and leave others to the delegate.

Currently, all Iceberg builtin procedures are under system namespace, which is guard by

iceberg/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/BaseCatalog.java

Line 51 in 81b3310

if (isSystemNamespace(namespace)) {

so I propose to add a simple rule to check the SQL before parsing

Do we have an example of someone else using the CALL syntax? Just wondering since we invented it for Iceberg specifically. I think this is fine either way, just a little disappointing we are losing a good error message for a bad one.

[pan3793]

Do we have an example of someone else using the CALL syntax?

@RussellSpitzer for now I have seen the following projects support CALL syntax too.

• Apache Hudi https://hudi.apache.org/docs/next/procedures
• Apache Paimon https://paimon.apache.org/docs/master/spark/procedures/

[RussellSpitzer]

Thanks @pan3793 for making sure things remain more compatible across the ecosystem. Thanks @huaxingao for reviewing!