You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/26 18:28:56 UTC
[GitHub] [iceberg] samredai opened a new pull request, #5360: [Python] Remove all base.py, use __init__.py where it works
samredai opened a new pull request, #5360:
URL: https://github.com/apache/iceberg/pull/5360
This is a PR to replace all of the `base.py` by either moving the contents into `__init__.py` or renaming them to something more meaningful.
As an example, the contents of `pyiceberg/io/base.py` has been moved to `pyiceberg/io/__init__.py`:
```py
from pyiceberg import io
dir(io)
# ['ABC', 'FileIO', 'InputFile', 'InputStream', 'OutputFile', 'OutputStream', 'Protocol', 'SEEK_SET', 'Union', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', 'abstractmethod', 'runtime_checkable']
```
To avoid circular imports in the expression sub-module, `base.py` was renamed `expression.py` and the public API is exposed via `__all__ = [...]` defined in the `__init__.py` for the expression module.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai closed pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai closed pull request #5360: [Python] Remove all base.py, use __init__.py where it works
URL: https://github.com/apache/iceberg/pull/5360
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930425088
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
+ BinaryLiteral,
+ BooleanLiteral,
+ DateLiteral,
+ DecimalLiteral,
+ DoubleLiteral,
+ FixedLiteral,
+ FloatLiteral,
+ LongLiteral,
+ StringLiteral,
+ TimeLiteral,
+ TimestampLiteral,
+ UUIDLiteral,
Review Comment:
Do we need the literal classes? I think the `literal` method should be enough. And we should also coerce values in predicates.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#issuecomment-1207469306
@rdblue I opened #5456 and #5457 which breaks out the `io` and `catalog` portion of this PR.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930425810
##########
python/pyiceberg/expression/expressions.py:
##########
@@ -489,88 +449,88 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundNotIn[T]:
return BoundNotIn(bound_ref, *tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
-class BoundEq(BoundPredicate[T]):
- def __invert__(self) -> BoundNotEq[T]:
- return BoundNotEq(self.term, *self.literals)
+class BoundEqual(BoundPredicate[T]):
Review Comment:
Can these renames happen in a separate PR?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930404656
##########
python/tests/catalog/test_catalog.py:
##########
@@ -20,43 +20,43 @@
List,
Optional,
Set,
+ Tuple,
Union,
)
import pytest
-from pyiceberg.catalog import Identifier, Properties
-from pyiceberg.catalog.base import Catalog
+from pyiceberg.catalog import Catalog
from pyiceberg.exceptions import (
AlreadyExistsError,
NamespaceNotEmptyError,
NoSuchNamespaceError,
NoSuchTableError,
)
from pyiceberg.schema import Schema
-from pyiceberg.table.base import Table
+from pyiceberg.table import Table
from pyiceberg.table.metadata import INITIAL_SPEC_ID
from pyiceberg.table.partitioning import PartitionSpec
class InMemoryCatalog(Catalog):
"""An in-memory catalog implementation for testing purposes."""
- __tables: Dict[Identifier, Table]
- __namespaces: Dict[Identifier, Properties]
+ __tables: Dict[Tuple[str, ...], Table]
Review Comment:
Unless I messed something up here, this was causing a circular import:
```
ImportError while loading conftest '/Users/sam/iceberg/python/tests/conftest.py'.
tests/conftest.py:55: in <module>
from tests.catalog.test_catalog import InMemoryCatalog
tests/catalog/test_catalog.py:28: in <module>
from pyiceberg.catalog import Catalog, Identifier, Properties
pyiceberg/catalog/__init__.py:24: in <module>
from pyiceberg.tableimport Table
pyiceberg/table/__init__.py:22: in <module>
from pyiceberg.catalog.base import Identifier
E ImportError: cannot import name 'Identifier' from partially initialized module 'pyiceberg.catalog' (most likely due to a circular import) (/Users/sam/iceberg/python/pyiceberg/catalog/__init__.py)
```
Essentially from [this](https://github.com/apache/iceberg/blob/master/python/pyiceberg/table/base.py#L22) line. If there's a way to avoid this and still use `__init__.py` I can update this. Although I do wonder how informative a type of `Identifier` is versus `Tuple[str, ...]`.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930426307
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
+ BinaryLiteral,
+ BooleanLiteral,
+ DateLiteral,
+ DecimalLiteral,
+ DoubleLiteral,
+ FixedLiteral,
+ FloatLiteral,
+ LongLiteral,
+ StringLiteral,
+ TimeLiteral,
+ TimestampLiteral,
+ UUIDLiteral,
+ literal,
+)
+
+__all__ = [
Review Comment:
I don't think literals really need to be exposed. Does that affect this decision?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#issuecomment-1203381169
@samredai, I think the next steps on this are to open PRs for the io and catalog modules. We need to think a bit more about what we want to expose from the expressions module. I think we may want something like the `Expressions` class in Java that hides creating references and literals.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930428620
##########
python/tests/catalog/test_catalog.py:
##########
@@ -20,43 +20,43 @@
List,
Optional,
Set,
+ Tuple,
Union,
)
import pytest
-from pyiceberg.catalog import Identifier, Properties
-from pyiceberg.catalog.base import Catalog
+from pyiceberg.catalog import Catalog
from pyiceberg.exceptions import (
AlreadyExistsError,
NamespaceNotEmptyError,
NoSuchNamespaceError,
NoSuchTableError,
)
from pyiceberg.schema import Schema
-from pyiceberg.table.base import Table
+from pyiceberg.table import Table
from pyiceberg.table.metadata import INITIAL_SPEC_ID
from pyiceberg.table.partitioning import PartitionSpec
class InMemoryCatalog(Catalog):
"""An in-memory catalog implementation for testing purposes."""
- __tables: Dict[Identifier, Table]
- __namespaces: Dict[Identifier, Properties]
+ __tables: Dict[Tuple[str, ...], Table]
Review Comment:
Maybe we should have a `typedef.py` file somewhere to handle these? I like the definitions.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930411183
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -1,20 +1,246 @@
-# 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
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Fixed!
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930362357
##########
python/tests/catalog/test_catalog.py:
##########
@@ -20,43 +20,43 @@
List,
Optional,
Set,
+ Tuple,
Union,
)
import pytest
-from pyiceberg.catalog import Identifier, Properties
-from pyiceberg.catalog.base import Catalog
+from pyiceberg.catalog import Catalog
from pyiceberg.exceptions import (
AlreadyExistsError,
NamespaceNotEmptyError,
NoSuchNamespaceError,
NoSuchTableError,
)
from pyiceberg.schema import Schema
-from pyiceberg.table.base import Table
+from pyiceberg.table import Table
from pyiceberg.table.metadata import INITIAL_SPEC_ID
from pyiceberg.table.partitioning import PartitionSpec
class InMemoryCatalog(Catalog):
"""An in-memory catalog implementation for testing purposes."""
- __tables: Dict[Identifier, Table]
- __namespaces: Dict[Identifier, Properties]
+ __tables: Dict[Tuple[str, ...], Table]
Review Comment:
Was this change intentional?
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -1,20 +1,246 @@
-# 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
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Some unwanted indentation here.
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
+ BinaryLiteral,
+ BooleanLiteral,
+ DateLiteral,
+ DecimalLiteral,
+ DoubleLiteral,
+ FixedLiteral,
+ FloatLiteral,
+ LongLiteral,
+ StringLiteral,
+ TimeLiteral,
+ TimestampLiteral,
+ UUIDLiteral,
+ literal,
+)
+
+__all__ = [
Review Comment:
I'm not a big fan of this mostly because it is a one-off and it is additional bookkeeping. Why not get rid of the whole module, and move all the expressions to `pyiceberg/expressions.py` and the literals to `pyiceberg/literals.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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930423741
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -14,7 +14,233 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Dict, Tuple
-Identifier = Tuple[str, ...]
-Properties = Dict[str, str]
Review Comment:
Why did these get removed? I think they are good shortcuts to keep.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930438386
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
+ BinaryLiteral,
+ BooleanLiteral,
+ DateLiteral,
+ DecimalLiteral,
+ DoubleLiteral,
+ FixedLiteral,
+ FloatLiteral,
+ LongLiteral,
+ StringLiteral,
+ TimeLiteral,
+ TimestampLiteral,
+ UUIDLiteral,
+ literal,
+)
+
+__all__ = [
Review Comment:
> I don't think literals really need to be exposed. Does that affect this decision?
If we replace the `expression` module with a single `expressions.py` file, should the contents of `literals.py` just be moved to the top of that file then? It would still be exposed if someone wanted to import it from `pyiceberg.expressions`.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930442441
##########
python/pyiceberg/table/__init__.py:
##########
@@ -1,16 +1,29 @@
-# 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
+# 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
+# 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.
+# 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 __future__ import annotations
+
+from abc import ABC
+
+
+class Table(ABC):
Review Comment:
There are still [other](https://github.com/apache/iceberg/tree/master/python/pyiceberg/table) files in the package directory such as `snapshots.py`, `partitioning.py`, etc.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930409480
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
+ BinaryLiteral,
+ BooleanLiteral,
+ DateLiteral,
+ DecimalLiteral,
+ DoubleLiteral,
+ FixedLiteral,
+ FloatLiteral,
+ LongLiteral,
+ StringLiteral,
+ TimeLiteral,
+ TimestampLiteral,
+ UUIDLiteral,
+ literal,
+)
+
+__all__ = [
Review Comment:
I thought about this but the expression related stuff does have enough of a logical grouping with things like literals, evaluators, all of the visitors, etc. so it seems valuable to have all of this contained in a submodule for context. I'm ok with either way here though so if nobody else chimes in I'll update 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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930428077
##########
python/pyiceberg/table/__init__.py:
##########
@@ -1,16 +1,29 @@
-# 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
+# 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
+# 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.
+# 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 __future__ import annotations
+
+from abc import ABC
+
+
+class Table(ABC):
Review Comment:
Since this is so small, should we move this all to `table.py` instead of a package directory?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930424311
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -14,7 +14,233 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Dict, Tuple
-Identifier = Tuple[str, ...]
-Properties = Dict[str, str]
+from __future__ import annotations
+
+from abc import ABC, abstractmethod
+
+from pyiceberg.schema import Schema
+from pyiceberg.table import Table
+from pyiceberg.table.partitioning import PartitionSpec
+
+
+class Catalog(ABC):
+ """Base Catalog for table operations like - create, drop, load, list and others.
+
+ The catalog table APIs accept a table identifier, which is fully classified table name. The identifier can be a string or
+ tuple of strings. If the identifier is a string, it is split into a tuple on '.'. If it is a tuple, it is used as-is.
+
+ The catalog namespace APIs follow a similar convention wherein they also accept a namespace identifier that can be a string
+ or tuple of strings.
+
+ Attributes:
+ name(str): Name of the catalog
+ properties(Dict[str, str]): Catalog properties
+ """
+
+ def __init__(self, name: str, properties: dict[str, str]):
Review Comment:
I don't think that the contents should change when moved. Can you make sure that this is as close as possible to the original?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930463449
##########
python/pyiceberg/io/__init__.py:
##########
@@ -14,3 +14,187 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+"""Base FileIO classes for implementing reading and writing table files
+
+The FileIO abstraction includes a subset of full filesystem implementations. Specifically,
+Iceberg needs to read or write a file at a given location (as a seekable stream), as well
+as check if a file exists. An implementation of the FileIO abstract base class is responsible
+for returning an InputFile instance, an OutputFile instance, and deleting a file given
+its location.
+"""
+from abc import ABC, abstractmethod
+from io import SEEK_SET
+from typing import Protocol, Union, runtime_checkable
Review Comment:
I'm not sure it's not being recognized but this was a simple `mv pyiceberg/io/base.py pyiceberg/io/__init__.py`
##########
python/pyiceberg/io/__init__.py:
##########
@@ -14,3 +14,187 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+"""Base FileIO classes for implementing reading and writing table files
+
+The FileIO abstraction includes a subset of full filesystem implementations. Specifically,
+Iceberg needs to read or write a file at a given location (as a seekable stream), as well
+as check if a file exists. An implementation of the FileIO abstract base class is responsible
+for returning an InputFile instance, an OutputFile instance, and deleting a file given
+its location.
+"""
+from abc import ABC, abstractmethod
+from io import SEEK_SET
+from typing import Protocol, Union, runtime_checkable
Review Comment:
I'm not sure why it's not being recognized but this was a simple `mv pyiceberg/io/base.py pyiceberg/io/__init__.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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930424634
##########
python/pyiceberg/expression/__init__.py:
##########
@@ -0,0 +1,83 @@
+# 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 pyiceberg.expression.expressions import (
+ And,
+ Equal,
+ GreaterThan,
+ GreaterThanEqual,
+ In,
+ IsNaN,
+ IsNull,
+ LessThan,
+ LessThanEqual,
+ Not,
+ NotEqual,
+ NotIn,
+ NotNaN,
+ NotNull,
+ Or,
+)
+from pyiceberg.expression.literals import (
+ AboveMax,
+ BelowMin,
Review Comment:
`AboveMax` and `BelowMin` don't need to be pulled in. Those are used in the binding process, but aren't needed afterward.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930427197
##########
python/pyiceberg/io/__init__.py:
##########
@@ -14,3 +14,187 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+"""Base FileIO classes for implementing reading and writing table files
+
+The FileIO abstraction includes a subset of full filesystem implementations. Specifically,
+Iceberg needs to read or write a file at a given location (as a seekable stream), as well
+as check if a file exists. An implementation of the FileIO abstract base class is responsible
+for returning an InputFile instance, an OutputFile instance, and deleting a file given
+its location.
+"""
+from abc import ABC, abstractmethod
+from io import SEEK_SET
+from typing import Protocol, Union, runtime_checkable
Review Comment:
Are there additional changes here that prevented git from recognizing that it was moved from `base.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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930427633
##########
python/pyiceberg/table/__init__.py:
##########
@@ -1,16 +1,29 @@
-# 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
+# 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
+# http://www.apache.org/licenses/LICENSE-2.0
Review Comment:
Unnecessary changes here 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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
samredai commented on PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#issuecomment-1196047728
@rdblue I removed all unnecessary changes
@Fokko in a separate [commit](https://github.com/apache/iceberg/pull/5360/commits/5374120c1cb94fc661aaf437b5c0e845a819df8c) I pulled flattened the expressions package directory into a top level `expressions.py` and `literals.py`. Let me know what you think.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5360: [Python] Remove all base.py, use __init__.py where it works
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5360:
URL: https://github.com/apache/iceberg/pull/5360#discussion_r930795783
##########
python/pyiceberg/io/__init__.py:
##########
@@ -14,3 +14,187 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+"""Base FileIO classes for implementing reading and writing table files
+
+The FileIO abstraction includes a subset of full filesystem implementations. Specifically,
+Iceberg needs to read or write a file at a given location (as a seekable stream), as well
+as check if a file exists. An implementation of the FileIO abstract base class is responsible
+for returning an InputFile instance, an OutputFile instance, and deleting a file given
+its location.
+"""
+from abc import ABC, abstractmethod
+from io import SEEK_SET
+from typing import Protocol, Union, runtime_checkable
Review Comment:
You could try `git mv pyiceberg/io/base.py pyiceberg/io/__init__.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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org