Skip to content

[CALCITE-6055] Customize handling of type name based on type system#3476

Draft
wnob wants to merge 1 commit intoapache:mainfrom
wnob:user-defined-types
Draft

[CALCITE-6055] Customize handling of type name based on type system#3476
wnob wants to merge 1 commit intoapache:mainfrom
wnob:user-defined-types

Conversation

@wnob
Copy link
Copy Markdown
Contributor

@wnob wnob commented Oct 18, 2023

Outline of the approach:

  • Treat TIMESTAMP, TIMESTAMP WITH LOCAL TIME ZONE, and DATETIME type names as user-defined types during parsing.
  • When deriving the types of these nodes, look for mappings in the schema model as before, but allow user-defined types to fall back on canonical types if no mapping is found, much like how SqlUnknownLiteral works. This is important so the TIMESTAMP and TIMESTAMP WITH LOCAL TIME ZONE types can still function when there is no UDT mapping for them.

There's still a lingering problem in how to unparse the SqlUserDefinedTypeNameSpec nodes. This is only relevant when a SqlNode AST is parsed and then unparsed without converting to RelNode form in between. I describe why I don't think this is a particularly important use-case in this Jira comment, but it's how the SqlParserTest and it's various subclasses operate. Looking for feedback on how to better address this:

  • The most obvious solution seems like implementing unparse() in a way similar to deriveType(): look up the UDT mapping and fall back on the standard types, thus preserving the existing default unparsing behavior for TIMESTAMP. However, this would require passing a SqlValidator to unparse(), which doesn't make a ton a sense.
  • We could instead ignore the UDT map during unparsing, but still try to match the type name to the standard SqlTypeName, so TIMESTAMP unparsing is unaffected by default. However, this would mess up unparsing a dialect like BigQuery, since BigQuery timestamps would unparse as TIMESTAMP WITH LOCAL TIME ZONE, and datetimes would unparse as `DATETIME` (with backticks).

There are myriad details that could be tweaked in either case, and possibly other approached I'm not thinking of yet.

@github-actions
Copy link
Copy Markdown

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant