[Kernel] Fix issue querying tables with spaces in the name#3291
Merged
Conversation
|
|
||
| /** | ||
| * Escapes the given string to be used as a partition value in the path. Basically this escapes | ||
| * - characters that can't be in a file path. E.g. `a\nb` will be escaped to `a%0Ab`. - |
Collaborator
There was a problem hiding this comment.
what's up with the - usage in this comment? is it a dot job? the "-" literal character?
Collaborator
Author
There was a problem hiding this comment.
auto-format. changed it to use the proper lists<ul>
| * <li><ul> | ||
| * <li>name: {@code path}, type: {@code string}, description: location of the file.</li> | ||
| * <li>name: {@code path}, type: {@code string}, description: location of the file. | ||
| * The path is a URI as specified by RFC 2396 URI Generic Syntax, which needs to be decoded |
Collaborator
There was a problem hiding this comment.
should the input to Table.forPath also be a String that represents a URI? Have we updated that documentation?
Collaborator
Author
There was a problem hiding this comment.
It is not at the moment, and we can't change that. If we want to take URI as an input we should be explicit about and and it should be another API, something like Table.forURI(URI tableURI).
The path here comes from Delta Log and stored as a URI in Delta Log according to the protocol. The name is path but it is actually a URI. Just updating the documentation to reflect that.
Before the next release, I will have a design decision to change the path string to URI everywhere else (basically in the Engine interfaces). Until then this is the fix.
Collaborator
There was a problem hiding this comment.
Thanks for explaining!
scottsand-db
approved these changes
Jun 24, 2024
vkorukanti
added a commit
to vkorukanti/delta
that referenced
this pull request
Aug 30, 2024
…3291) ## Description Currently, Kernel uses a mix of path (file system path) or URI (in string format) in API interfaces, which causes confusion and bugs. Context: Path refers to a file system path which could have some characters that should be escaped when converted to URI E.g. path: `s3:/bucket/path to file/`, URI for the same path: `s3:/bucket/path%20to%20file/` Make it uniform everywhere to just use the paths (file system path). ## How was this patch tested? Additional tests with table path containing spaces.
mblanco-denodo
added a commit
to mblanco-denodo/presto
that referenced
this pull request
Dec 16, 2025
## Description Upgrade delta-kernel-api and delta-kernel-defaults library to version 3.3.2 ## Motivation and Context Upgrading those libraries will make us able to support future improvements in the connector, like support for deletion vectors, type widening, varian type... . ## Impact Library bug fixes from 3.2.0 and ability to support new features. In the 3.2.1 version, this bug was fixed: delta-io/delta#3291 so this previous bugfix has been undone as it is not needed anymore: prestodb#26397 ## Test Plan There already exist unit tests. Since this is only a library version upgrade, passing the unit tests should be our target. ## Release Notes ``` == NO RELEASE NOTE == ```
mblanco-denodo
added a commit
to mblanco-denodo/presto
that referenced
this pull request
Dec 17, 2025
## Description Upgrade delta-kernel-api and delta-kernel-defaults library to version 3.3.2 ## Motivation and Context Upgrading those libraries will make us able to support future improvements in the connector, like support for deletion vectors, type widening, varian type... . ## Impact Library bug fixes from 3.2.0 and ability to support new features. In the 3.2.1 version, this bug was fixed: delta-io/delta#3291 so this previous bugfix has been undone as it is not needed anymore: prestodb#26397 ## Test Plan There already exist unit tests. Since this is only a library version upgrade, passing the unit tests should be our target. ## Release Notes ``` == NO RELEASE NOTE == ```
mblanco-denodo
added a commit
to mblanco-denodo/presto
that referenced
this pull request
Dec 18, 2025
## Description Upgrade delta-kernel-api and delta-kernel-defaults library to version 3.3.2 ## Motivation and Context Upgrading those libraries will make us able to support future improvements in the connector, like support for deletion vectors, type widening, varian type... . ## Impact Library bug fixes from 3.2.0 and ability to support new features. In the 3.2.1 version, this bug was fixed: delta-io/delta#3291 so this previous bugfix has been undone as it is not needed anymore: prestodb#26397 ## Test Plan There already exist unit tests. Since this is only a library version upgrade, passing the unit tests should be our target. ## Release Notes ``` == NO RELEASE NOTE == ```
hantangwangd
pushed a commit
to prestodb/presto
that referenced
this pull request
Dec 25, 2025
…26814) ## Description Upgrade delta-kernel-api and delta-kernel-defaults library to version 3.3.2 ## Motivation and Context Upgrading those libraries will make us able to support future improvements in the connector, like support for deletion vectors, type widening, variant type... . ## Impact Library bug fixes from 3.2.0 and ability to support new features. In the 3.2.1 version, this bug was fixed: delta-io/delta#3291 so this previous bugfix has been undone as it is not needed anymore: #26397 ## Test Plan There already exist unit tests. Since this is only a library version upgrade, passing the unit tests should be our target. ## Release Notes ``` == NO RELEASE NOTE == ```
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
(Stacked on top of #3289 and #3290 )
Currently, Kernel uses a mix of path (file system path) or URI (in string format) in API interfaces, which causes confusion and bugs.
Context:
Path refers to a file system path which could have some characters that should be escaped when converted to URI
E.g. path:
s3:/bucket/path to file/, URI for the same path:s3:/bucket/path%20to%20file/Make it uniform everywhere to just use the paths (file system path).
How was this patch tested?
Additional tests with table path containing spaces.