Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing fails when there isn't default schema #528

Open
Chaho12 opened this issue Oct 17, 2024 · 5 comments · May be fixed by #531
Open

Parsing fails when there isn't default schema #528

Chaho12 opened this issue Oct 17, 2024 · 5 comments · May be fixed by #531
Labels
bug Something isn't working

Comments

@Chaho12
Copy link
Member

Chaho12 commented Oct 17, 2024

I notice there is error on parsing TrinoQueryProperties file, when there isn’t default schema set while using WITH clause
sql is sth like this where it has default catalog, but not default schema for temporary table created with WITH.

WITH ctr AS (
  SELECT sscode,
         SUM(sc) AS sc,
  FROM mysql_laim_data.lboard_data.ctr
  WHERE ...
), t_summary_by_sscode AS (
SELECT ...
FROM
(
  SELECT *
  FROM ctr LEFT OUTER JOIN ctr_before_one
  USING (sscode)
  ) LEFT OUTER JOIN ctr_before_two
  USING (sscode)
)

SELECT sscode,
       QC
FROM t_summary_by_sscode
...

Image
Image

@Chaho12 Chaho12 added the bug Something isn't working label Oct 17, 2024
@mosabua
Copy link
Member

mosabua commented Oct 17, 2024

Default catalog and schema are provided by the client tool if they are set.. they have nothing to do with the actual query. So in that sense the fact that schema is empty is perfectly fine.

I would consider this to work as designed.

@Chaho12
Copy link
Member Author

Chaho12 commented Oct 18, 2024

Yes i agree that schema can be empty.
But the issue is the if schema is empty, current code would throw unsetDefaultExceptionSupplier because default schema is empty.
Throwing error is not what we expect as SQL could be valid.

@Chaho12
Copy link
Member Author

Chaho12 commented Oct 18, 2024

I am thinking sth like this where we have a set that has list of temporary tables that is created when parsing to a tree.

This part of code is where it builds tree i assume?
https://github.com/trinodb/trino-gateway/blob/main/gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java#L202-L203

private QualifiedName qualifyName(QualifiedName table)
            throws RequestParsingException
{
    List<String> tableParts = table.getParts();
    return switch (tableParts.size()) {
        case 1 -> {
            String tableName = tableParts.getFirst();
            if (temporaryTables.contains(tableName)) {
                // For temporary tables, use only the table name
                yield QualifiedName.of(tableName);
            } else {
                // For non-temporary tables, use default catalog and schema if available
                yield QualifiedName.of(
                    defaultCatalog.orElseThrow(this::unsetDefaultExceptionSupplier),
                    defaultSchema.orElse(""), // Use empty string if default schema is not set
                    tableName
                );
            }
        }
        case 2 -> QualifiedName.of(defaultCatalog.orElseThrow(this::unsetDefaultExceptionSupplier), tableParts.getFirst(), tableParts.get(1));
        case 3 -> QualifiedName.of(tableParts.getFirst(), tableParts.get(1), tableParts.get(2));
        default -> throw new RequestParsingException("Unexpected table name: " + table.getParts());
    };
}

@mosabua
Copy link
Member

mosabua commented Oct 18, 2024

Have not look at your code but yes .. query parsing should work with

  • empty default catalog and empty default schema
  • values in both

If that is not the case, thats a bug and you should send a PR to fix that for sure.

I am not so certain if behavior is defined in Trino for

  • default catalog but no schema
  • default schema but no catalog

so we should find out. It might be related to the USE comment where you have to specify both.

@mosabua
Copy link
Member

mosabua commented Oct 18, 2024

Also note .. in the CLI you can either specify catalog and schema, or just catalog, but no just schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants