API support for case-(in)sensitive table names #1369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryzhyk
force-pushed
the
case-insensitive-tables
branch
from
af196dc
to
b5bbb51
Compare
|
@Karakatiza666 , I need your help wrapping up this PR. It requires referring to case-sensitive relation names in quotes in the API, including |
4 tasks
ryzhyk
added
Web Console
mihaibudiu
approved these changes
Feb 1, 2024
| name.to_lowercase() | ||
| }; | ||
|
|
||
| self.input_collection_handles.insert(name, handle); |
There was a problem hiding this comment.
can this fail?
does this overwrite a previously existing name?
There was a problem hiding this comment.
yes, there's no checking for duplicates at the moment. I'll add that.
|
|
||
| /// A SQL table or view. It has a name and a list of fields. | ||
| /// | ||
| /// Matches the Calcite JSON format. |
There was a problem hiding this comment.
Calcite or SQL compiler?
There was a problem hiding this comment.
I didn't write this comment, but I'm pretty sure it's the SQL compiler.
| /// - `VARCHAR(255)` sets precision to `255`. | ||
| /// - `BIGINT`, `DATE`, `FLOAT`, `DOUBLE`, `GEOMETRY`, etc. sets precision | ||
| /// to None | ||
| /// - `TIME`, `TIMESTAMP` set precision to `0`. |
There was a problem hiding this comment.
This looks wrong, I think that Calcite only works with precision "3" for TIMESTAMP.
Maybe this is a bug we should look into.
There was a problem hiding this comment.
I just copied this code into a different location, haven't touched it
There was a problem hiding this comment.
at the time, I verified this by manually creating these types and see what I get back from the compiler
lalithsuresh
approved these changes
Feb 1, 2024
ryzhyk
force-pushed
the
case-insensitive-tables
branch
2 times, most recently
from
8929ad2
to
655a0ab
Compare
ryzhyk
force-pushed
the
case-insensitive-tables
branch
from
a187b02
to
0982fff
Compare
Upcoming commits will use these types in the `adapters` crate, which will need to manipulate program schema. Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
This field was added a while ago by the compiler, but hasn't been used. We are going to start using it in an upcoming commit. Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
The SQL compiler now passes SQL table schema descriptions in the JSON format when registering tables and views in the catalog. Upcoming commits make use of the schema in two ways: 1. Extract case sensitivity information from the schema in order to correctly handle table names in API calls and in pipeline configuration. 2. Use the schema to generate table schema for data formats that require it (initially, Kafka Connect's JSON format). Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
This column was recently added by the compiler. Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
Previously our API insisted on referring to tables and views in upper case. This manifested in two places: `/ingress/TABLE_NAME` and `/egress/VIEW_NAME` API endpoints and in connection configuration, where the user must specify table or view name. In this commit we remove this limitation and allow relations to be addressed using any case. More specifically: - Case-insensitive relation names, i.e., relations declared without quotes in SQL, can be addressed in UPPERCASE, lowercase, or MiXeD CaSe, e.g., `/ingress/TABLE`, `/ingress/table`, and `/ingress/TaBlE` are all accepted. - Case-sensitive relations declared with quotes, e.g., `create table "Foo"(...)` or `create view "BAR" as ...` must be addressed in the same case as their declaration by putting their names in quotes: `/ingress/"Foo"`, `/egress/"BAR"`. Addresses issue #51. Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
Signed-off-by: George <bulakh.96@gmail.com>
ryzhyk
force-pushed
the
case-insensitive-tables
branch
from
0982fff
to
26ae774
Compare
Signed-off-by: George <bulakh.96@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously our API insisted on referring to tables and views in upper case.
This manifested in two places:
/ingress/TABLE_NAMEand/egress/VIEW_NAMEAPI endpoints and in connection configuration, where the user must specify
table or view name.
In this commit we remove this limitation and allow relations to be addressed
using any case. More specifically:
Case-insensitive relation names, i.e., relations declared without
quotes in SQL, can be addressed in UPPERCASE, lowercase, or MiXeD
CaSe, e.g.,
/ingress/TABLE,/ingress/table, and/ingress/TaBlEare allaccepted.
Case-sensitive relations declared with quotes, e.g.,
create table "Foo"(...)orcreate view "BAR" as ...must beaddressed in the same case as their declaration by putting their
names in quotes:
/ingress/"Foo",/egress/"BAR".Addresses issue #51.
Is this a user-visible change (yes/no): yes