You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "discivigour (via GitHub)" <gi...@apache.org> on 2025/12/12 04:41:26 UTC
[PR] [python] Refactor read and write options [paimon]
discivigour opened a new pull request, #6808:
URL: https://github.com/apache/paimon/pull/6808
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### Purpose
In the current code, the default values of various options are being strolled, and there are also problems such as type non-standardization (int -> str). Currently, pr has refactored the reading and writing of options.
- CoreOptions and Options for internal use.
- dict for Interaction with the outside.
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on PR #6808:
URL: https://github.com/apache/paimon/pull/6808#issuecomment-3674259275
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "discivigour (via GitHub)" <gi...@apache.org>.
discivigour commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2616998740
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -158,17 +159,17 @@ def add_options(self, options: dict):
Review Comment:
right, added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2613958513
##########
paimon-python/pypaimon/common/options/options.py:
##########
@@ -0,0 +1,58 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+from pypaimon.common.options import ConfigOption
+from pypaimon.common.options.options_utils import OptionsUtils
+
+
+class Options:
+ def __init__(self, data: dict):
+ self.data = data
+
+ @classmethod
+ def from_none(cls):
+ return cls({})
+
+ def to_map(self) -> dict:
+ return self.data
+
+ def get(self, key: ConfigOption, default=None):
+ """
Review Comment:
I perfer to support both str and ConfigOption here. If ConfigOption only, can you please test whether these code in file_io.py works will, client_kwargs['endpoint_override'] = self.properties.get(OssOptions.OSS_ENDPOINT) and other cases like this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2634211279
##########
paimon-python/pypaimon/catalog/filesystem_catalog.py:
##########
@@ -64,7 +65,7 @@ def create_database(self, name: str, ignore_if_exists: bool, properties: Optiona
def get_table(self, identifier: Union[str, Identifier]) -> Table:
if not isinstance(identifier, Identifier):
identifier = Identifier.from_string(identifier)
- if CoreOptions.SCAN_FALLBACK_BRANCH in self.catalog_options:
+ if self.catalog_options.contains(CoreOptions.SCAN_FALLBACK_BRANCH):
raise ValueError(f"Unsupported CoreOption {CoreOptions.SCAN_FALLBACK_BRANCH}")
table_path = self.get_table_path(identifier)
table_schema = self.get_table_schema(identifier)
Review Comment:
check `schema.options.get(CoreOptions.AUTO_CREATE):` in create_table here, schema.options is dict of str, but CoreOptions.AUTO_CREATE is config option, add test case to cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2617111702
##########
paimon-python/pypaimon/catalog/rest/rest_token_file_io.py:
##########
@@ -59,7 +60,7 @@ def __setstate__(self, state):
def _initialize_oss_fs(self, path) -> FileSystem:
self.try_to_refresh_token()
merged_token = self._merge_token_with_catalog_options(self.token.token)
- self.properties.update(merged_token)
+ self.properties.data.update(merged_token)
return super()._initialize_oss_fs(path)
def _merge_token_with_catalog_options(self, token: dict) -> dict:
Review Comment:
check here too, key should be string if merge_token is a dict of string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2617111702
##########
paimon-python/pypaimon/catalog/rest/rest_token_file_io.py:
##########
@@ -59,7 +60,7 @@ def __setstate__(self, state):
def _initialize_oss_fs(self, path) -> FileSystem:
self.try_to_refresh_token()
merged_token = self._merge_token_with_catalog_options(self.token.token)
- self.properties.update(merged_token)
+ self.properties.data.update(merged_token)
return super()._initialize_oss_fs(path)
def _merge_token_with_catalog_options(self, token: dict) -> dict:
Review Comment:
check here too, key should be string of merge_token is a dict of string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2613992696
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -142,7 +143,7 @@ def create_row_key_extractor(self) -> RowKeyExtractor:
def copy(self, options: dict) -> 'FileStoreTable':
Review Comment:
please double check here, if this should be updated to options as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2634268488
##########
paimon-python/pypaimon/common/file_io.py:
##########
@@ -35,7 +37,7 @@
class FileIO:
- def __init__(self, path: str, catalog_options: dict):
+ def __init__(self, path: str, catalog_options: Options):
self.properties = catalog_options
Review Comment:
check the test case which invoke FileIO. such as `blob_test.py`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "discivigour (via GitHub)" <gi...@apache.org>.
discivigour commented on PR #6808:
URL: https://github.com/apache/paimon/pull/6808#issuecomment-3660141110
> @discivigour Please rebase latest master.
>
> cc @XiaoHongbo-Hope to take a look.
rebased
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on PR #6808:
URL: https://github.com/apache/paimon/pull/6808#issuecomment-3646063893
`if OssOptions.OSS_ACCESS_KEY_ID in file_io.properties:` may fail if options is not iterable, maybe we can use contains instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2613958513
##########
paimon-python/pypaimon/common/options/options.py:
##########
@@ -0,0 +1,58 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+from pypaimon.common.options import ConfigOption
+from pypaimon.common.options.options_utils import OptionsUtils
+
+
+class Options:
+ def __init__(self, data: dict):
+ self.data = data
+
+ @classmethod
+ def from_none(cls):
+ return cls({})
+
+ def to_map(self) -> dict:
+ return self.data
+
+ def get(self, key: ConfigOption, default=None):
+ """
Review Comment:
I perfer to support both str and ConfigOption here. If ConfigOption only, can you please test whether these code in file_io.py works will, client_kwargs['endpoint_override'] = self.properties.get(OssOptions.OSS_ENDPOINT) and other cases like this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2633563131
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -154,21 +155,21 @@ def copy(self, options: dict) -> 'FileStoreTable':
def add_options(self, options: dict):
for key, value in options.items():
- self.options[key] = value
+ self.options.options.data[key] = value
Review Comment:
a little strange here, does add_options mean copy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2613992696
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -142,7 +143,7 @@ def create_row_key_extractor(self) -> RowKeyExtractor:
def copy(self, options: dict) -> 'FileStoreTable':
Review Comment:
please double check here, if this should be updated to options as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #6808:
URL: https://github.com/apache/paimon/pull/6808#issuecomment-3659099848
@discivigour Please rebase latest master.
cc @XiaoHongbo-Hope to take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2634211279
##########
paimon-python/pypaimon/catalog/filesystem_catalog.py:
##########
@@ -64,7 +65,7 @@ def create_database(self, name: str, ignore_if_exists: bool, properties: Optiona
def get_table(self, identifier: Union[str, Identifier]) -> Table:
if not isinstance(identifier, Identifier):
identifier = Identifier.from_string(identifier)
- if CoreOptions.SCAN_FALLBACK_BRANCH in self.catalog_options:
+ if self.catalog_options.contains(CoreOptions.SCAN_FALLBACK_BRANCH):
raise ValueError(f"Unsupported CoreOption {CoreOptions.SCAN_FALLBACK_BRANCH}")
table_path = self.get_table_path(identifier)
table_schema = self.get_table_schema(identifier)
Review Comment:
check `schema.options.get(CoreOptions.AUTO_CREATE):` in create_table here, schema.options is dict of str, but CoreOptions.AUTO_CREATE is config option, add test case to cover it. `schema.options.get(CoreOptions.TYPE)` too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2634232836
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -140,9 +141,9 @@ def create_row_key_extractor(self) -> RowKeyExtractor:
raise ValueError(f"Unsupported bucket mode: {bucket_mode}")
def copy(self, options: dict) -> 'FileStoreTable':
- if CoreOptions.BUCKET in options and options.get(CoreOptions.BUCKET) != self.options.get(CoreOptions.BUCKET):
+ if CoreOptions.BUCKET.key() in options and options.get(CoreOptions.BUCKET.key()) != self.options.bucket():
raise ValueError("Cannot change bucket number")
Review Comment:
options is a dict of str and self.options.bucket() will return int, will this work well? Can you add a case or assertion to cover here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2617098356
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -140,9 +141,9 @@ def create_row_key_extractor(self) -> RowKeyExtractor:
raise ValueError(f"Unsupported bucket mode: {bucket_mode}")
def copy(self, options: dict) -> 'FileStoreTable':
- if CoreOptions.BUCKET in options and options.get(CoreOptions.BUCKET) != self.options.get(CoreOptions.BUCKET):
+ if CoreOptions.BUCKET in options and options.get(CoreOptions.BUCKET.key()) != self.options.bucket():
raise ValueError("Cannot change bucket number")
Review Comment:
CoreOptions.BUCKET should be a ConfigOption, but here options is a dict of string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2633563131
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -154,21 +155,21 @@ def copy(self, options: dict) -> 'FileStoreTable':
def add_options(self, options: dict):
for key, value in options.items():
- self.options[key] = value
+ self.options.options.data[key] = value
Review Comment:
a little stranger here, does add_options mean copy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #6808:
URL: https://github.com/apache/paimon/pull/6808
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on PR #6808:
URL: https://github.com/apache/paimon/pull/6808#issuecomment-3660011146
> @discivigour Please rebase latest master.
>
> cc @XiaoHongbo-Hope to take a look.
👌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [python] Refactor read and write options [paimon]
Posted by "XiaoHongbo-Hope (via GitHub)" <gi...@apache.org>.
XiaoHongbo-Hope commented on code in PR #6808:
URL: https://github.com/apache/paimon/pull/6808#discussion_r2614017547
##########
paimon-python/pypaimon/table/file_store_table.py:
##########
@@ -158,17 +159,17 @@ def add_options(self, options: dict):
Review Comment:
please double check here, CoreOptions does not have setitem method, this may fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org