-
Notifications
You must be signed in to change notification settings - Fork 332
Allow Polaris CLI to set property values that contain '=' #1003
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
Allow Polaris CLI to set property values that contain '=' #1003
Conversation
Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='. This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string.
|
I think this is reasonable, but what about |
Yeah it's worth thinking about where to draw the line between "sophisticated" scenarios and ones that work straight out of the box. As you mention, cases that might require '=' in the key could still go all the way to the Python client. In practical terms, there does seem to be precedent for being able to specify kv lists as the value of a property and values that use base64 encodings that would include '='. In both these cases, the '=' is present in the string precisely because of the value not being from low-cardinality enumeration of possible values. It does seem that non-enumerated keys would be much rarer, since by nature such a situation would only occur if the consuming code iterates over all keys instead of performing a point-get of a key. |
|
Non-enumerated keys are certainly rarer, but also impossible to detect if
we make this change.
…On Mon, Feb 17, 2025 at 7:18 PM Dennis Huo ***@***.***> wrote:
I think this is reasonable, but what about =s in the key? I initially
decided to just restrict = altogether because you can always use the
Python client instead of the CLI for more sophisticated scenarios like
this. Another alternative would be to support JSON or some other syntax
that lets us escape special characters.
Yeah it's worth thinking about where to draw the line between
"sophisticated" scenarios and ones that work straight out of the box. As
you mention, cases that might require '=' in the key could still go all the
way to the Python client.
In practical terms, there does seem to be precedent for being able to
specify kv lists as the value of a property and values that use base64
encodings that would include '='. In both these cases, the '=' is present
in the string precisely because of the value not being from low-cardinality
enumeration of possible values. It does seem that non-enumerated keys would
be much rarer, since by nature such a situation would only occur if the
consuming code iterates over all keys instead of performing a point-get of
a key.
—
Reply to this email directly, view it on GitHub
<#1003 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFRE3SGNQLRWUALW7ACYZJD2QKRBTAVCNFSM6AAAAABXDWUDTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUGUYTINBUGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
[image: dennishuo]*dennishuo* left a comment (apache/polaris#1003)
<#1003 (comment)>
I think this is reasonable, but what about =s in the key? I initially
decided to just restrict = altogether because you can always use the
Python client instead of the CLI for more sophisticated scenarios like
this. Another alternative would be to support JSON or some other syntax
that lets us escape special characters.
Yeah it's worth thinking about where to draw the line between
"sophisticated" scenarios and ones that work straight out of the box. As
you mention, cases that might require '=' in the key could still go all the
way to the Python client.
In practical terms, there does seem to be precedent for being able to
specify kv lists as the value of a property and values that use base64
encodings that would include '='. In both these cases, the '=' is present
in the string precisely because of the value not being from low-cardinality
enumeration of possible values. It does seem that non-enumerated keys would
be much rarer, since by nature such a situation would only occur if the
consuming code iterates over all keys instead of performing a point-get of
a key.
—
Reply to this email directly, view it on GitHub
<#1003 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFRE3SGNQLRWUALW7ACYZJD2QKRBTAVCNFSM6AAAAABXDWUDTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUGUYTINBUGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
I think we should just go for consistency with common tools, and everything else by default behaves with "split at first '='": |
eric-maynard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior is useful to users, and I'm convinced by the argument that splitting on the first = is enough of a standard. Like I said elsewhere, users who need other behavior can always use the python client.
* Publish proposals and roadmap on the website (apache#1043) * Allow Polaris CLI to set property values that contain '=' (apache#1003) Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='. This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string. * main: Update postgres Docker tag to v17.4 (apache#1049) * main: Update dependency org.junit:junit-bom to v5.12.0 (apache#1045) * Revert "Remove CallContext and its ThreadLocal usage" (apache#1000) * revert b84f462 * resolve conflicts * autolint * autolint * ...not possible to automatically add a synthetic no-args constructor to an unproxyable bean class. * fix * fixed non-quarkus build issues * fixed non-quarkus build issues * autolint * more di issues * add more producers * add another producer * autolint * change callcontext usage in FileIOFactoryTest * stablize tests * autolint * fix a test * refactor TableCleanupTaskHandlerTest * another fix, still npe * autolint * only task cleanup is problematic * autolint * Stabilize more tests * autolint * Avoid propagating old callcontexts across TestServices instances * testservices update * autolint * Revert "autolint" This reverts commit 66a7f22. * Revert "testservices update" This reverts commit 897a5a1. * changes to taskexecutorimpl * autolint --------- Co-authored-by: Dennis Huo <[email protected]> * Enable console color when running Polaris with Gradle (apache#977) * CLI: add subcommand access for principals (apache#1019) * Access subcommand access for principals * Access subcommand access for principals * Fix getting start spark (apache#1054) * Use correct link format for adoc and update menu title for proposals (apache#1051) * Add a debug log with the full exception (apache#1023) * clean up * spotless * clean up * refactor message * no CVEM * change log levels * revert polaris-java dep change * add testImplementation * uppercase * Remove all tightly coupled EntityCache dependencies in the main persistence stack (apache#1055) Remove the EntityCacheEntry wrapper since the timestamp fields were never used anyways; instead the underlying Caffeine cache transparently handles access times, and the types of entries we cache are simply the existing ResolvedPolarisEntity. Remove interactions of business logic with explicit "cache entries", instead operating on ResolvedPolarisEntity. Fix the equals()/hashCode() behavior of PolarisEntity to be compatible with PolarisBaseEntity as intended. Improve code comments to explain the (current) relationship between PolarisEntity and PolarisBaseEntity, and clarify the behaviors in Resolver.java. Fully remove the PolarisRemoteCache interface and its methods. Add different methods that aren't cache-specific instead. * Remove PolarisMetaStoreSession from FileIOFactory/FileIOUtil in favor of CallContext (apache#1057) This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory. This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest. * Cleanup Iceberg Manifest list files on Purge (apache#1039) * Fix small quickstart typo (apache#1061) * Enable rat check on md files and fix the existing ones (apache#1050) * main: Update dependency boto3 to v1.37.0 (apache#1052) * main: Update dependency software.amazon.awssdk:bom to v2.30.27 (apache#1053) * Integration tests: improve support for temporary folders (apache#987) 3 integration tests currently require a temporary folder for various purposes. The folders are generally hard-coded to some local filesystem location, which makes those tests impossible to execute against a remote Polaris instance. This PR aims at fixing this situation by leveraging a new optional environment variable that points to a location that is accessible by both the server and the client running the tests. This would typically be an S3 bucket, for example, but could also be a shared volume mounted on both machines at the same mount point. If this environment variable is not present, then local execution is assumed and a temporary directory managed by JUnit is used instead. * build-logic: exclude generated code from errorprone (apache#1035) * build-logic: let javadoc depend on jandex (apache#1034) * Add Prashant in the collaborators list (apache#1064) * main: Update dependency org.slf4j:slf4j-api to v2.0.17 (apache#1065) --------- Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: MonkeyCanCode <[email protected]> Co-authored-by: Andrew Guterman <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Robert Stupp <[email protected]>
Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='.
This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string.