You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/15 00:27:40 UTC
[GitHub] [beam] robertwb opened a new pull request #13122: [BEAM-9547] More complete indexing capabilities.
robertwb opened a new pull request #13122:
URL: https://github.com/apache/beam/pull/13122
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb merged pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robertwb merged pull request #13122:
URL: https://github.com/apache/beam/pull/13122
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13122:
URL: https://github.com/apache/beam/pull/13122#issuecomment-709614158
Sorry, I forgot you were busy with the release. I was just trying to spread the reviews out. Thanks for taking a look, Brian.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13122:
URL: https://github.com/apache/beam/pull/13122#issuecomment-709585382
Yeah I can take a look today or tomorrow
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13122:
URL: https://github.com/apache/beam/pull/13122#discussion_r506755598
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -32,9 +32,74 @@ def __array__(self, dtype=None):
raise frame_base.WontImplementError(
'Conversion to a non-deferred a numpy array.')
+ get = frame_base.not_implemented_method('get')
+
@frame_base.DeferredFrame._register_for(pd.Series)
class DeferredSeries(DeferredDataFrameOrSeries):
+ def __getitem__(self, key):
+ if _is_null_slice(key) or key is Ellipsis:
+ return self
+
+ elif (isinstance(key, int) or _is_integer_slice(key)
+ ) and self._expr.proxy().index._should_fallback_to_positional():
+ raise frame_base.WontImplementError('order sensitive')
+
+ elif isinstance(key, slice) or callable(key):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df: df[key],
+ [self._expr],
+ requires_partition_by=partitionings.Nothing(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif isinstance(key, DeferredSeries):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df, indexer: df[indexer],
+ [self._expr, key._expr],
+ requires_partition_by=partitionings.Index(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif pd.core.series.is_iterator(key) or pd.core.common.is_bool_indexer(key):
+ raise frame_base.WontImplementError('order sensitive')
+
+ else:
+ # We could consider returning a deferred scalar, but that might
+ # be more surprising than a clear error.
+ raise frame_base.WontImplementError('non-deferred')
+
+ if isinstance(key, frame_base.DeferredBase):
+ # Fail early if key is a DeferredBase as it interacts surprisingly with
+ # key in self._expr.proxy().columns
+ raise NotImplementedError(
+ "Indexing with a deferred frame is not yet supported. Consider "
+ "using df.loc[...]")
+
+ if isinstance(key, slice):
+ types = set([type(key.start), type(key.stop), type(key.step)])
+ if types == {type(None)}:
+ # Empty slice is just a copy.
+ return frame_base.DeferredFrame.wrap(self._expr)
+ elif types in [{int}, {type(None), int}]:
Review comment:
You could use `_is_null_slice` and `_is_integer_slice` here for clarity.
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -32,9 +32,74 @@ def __array__(self, dtype=None):
raise frame_base.WontImplementError(
'Conversion to a non-deferred a numpy array.')
+ get = frame_base.not_implemented_method('get')
+
@frame_base.DeferredFrame._register_for(pd.Series)
class DeferredSeries(DeferredDataFrameOrSeries):
+ def __getitem__(self, key):
+ if _is_null_slice(key) or key is Ellipsis:
+ return self
+
+ elif (isinstance(key, int) or _is_integer_slice(key)
+ ) and self._expr.proxy().index._should_fallback_to_positional():
+ raise frame_base.WontImplementError('order sensitive')
+
+ elif isinstance(key, slice) or callable(key):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df: df[key],
+ [self._expr],
+ requires_partition_by=partitionings.Nothing(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif isinstance(key, DeferredSeries):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df, indexer: df[indexer],
+ [self._expr, key._expr],
+ requires_partition_by=partitionings.Index(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif pd.core.series.is_iterator(key) or pd.core.common.is_bool_indexer(key):
+ raise frame_base.WontImplementError('order sensitive')
+
+ else:
+ # We could consider returning a deferred scalar, but that might
+ # be more surprising than a clear error.
+ raise frame_base.WontImplementError('non-deferred')
+
+ if isinstance(key, frame_base.DeferredBase):
+ # Fail early if key is a DeferredBase as it interacts surprisingly with
+ # key in self._expr.proxy().columns
+ raise NotImplementedError(
+ "Indexing with a deferred frame is not yet supported. Consider "
+ "using df.loc[...]")
+
+ if isinstance(key, slice):
+ types = set([type(key.start), type(key.stop), type(key.step)])
+ if types == {type(None)}:
+ # Empty slice is just a copy.
+ return frame_base.DeferredFrame.wrap(self._expr)
+ elif types in [{int}, {type(None), int}]:
+ # This depends on the contents of the index.
+ raise frame_base.WontImplementError(
+ 'Use iloc or loc with integer slices.')
Review comment:
Doesn't `iloc` only work for a null slice? I think we should just direct users to `loc` iff the proxy has an integer index. If it has a non-integer index we could tell the user they're going to have a bad time (maybe eventually it would link to some documentation about the dangers of integer-location based indexing in DataframeTransform).
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -453,17 +518,31 @@ def __getattr__(self, name):
def __getitem__(self, key):
# TODO: Replicate pd.DataFrame.__getitem__ logic
- if isinstance(key, frame_base.DeferredBase):
+ if isinstance(key, DeferredSeries) and key._expr.proxy().dtype == bool:
Review comment:
Should we have this logic in `DeferredSeries.__getitem__` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a change in pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #13122:
URL: https://github.com/apache/beam/pull/13122#discussion_r507933364
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -32,9 +32,74 @@ def __array__(self, dtype=None):
raise frame_base.WontImplementError(
'Conversion to a non-deferred a numpy array.')
+ get = frame_base.not_implemented_method('get')
+
@frame_base.DeferredFrame._register_for(pd.Series)
class DeferredSeries(DeferredDataFrameOrSeries):
+ def __getitem__(self, key):
+ if _is_null_slice(key) or key is Ellipsis:
+ return self
+
+ elif (isinstance(key, int) or _is_integer_slice(key)
+ ) and self._expr.proxy().index._should_fallback_to_positional():
+ raise frame_base.WontImplementError('order sensitive')
+
+ elif isinstance(key, slice) or callable(key):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df: df[key],
+ [self._expr],
+ requires_partition_by=partitionings.Nothing(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif isinstance(key, DeferredSeries):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df, indexer: df[indexer],
+ [self._expr, key._expr],
+ requires_partition_by=partitionings.Index(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif pd.core.series.is_iterator(key) or pd.core.common.is_bool_indexer(key):
+ raise frame_base.WontImplementError('order sensitive')
+
+ else:
+ # We could consider returning a deferred scalar, but that might
+ # be more surprising than a clear error.
+ raise frame_base.WontImplementError('non-deferred')
+
+ if isinstance(key, frame_base.DeferredBase):
+ # Fail early if key is a DeferredBase as it interacts surprisingly with
+ # key in self._expr.proxy().columns
+ raise NotImplementedError(
+ "Indexing with a deferred frame is not yet supported. Consider "
+ "using df.loc[...]")
+
+ if isinstance(key, slice):
+ types = set([type(key.start), type(key.stop), type(key.step)])
+ if types == {type(None)}:
+ # Empty slice is just a copy.
+ return frame_base.DeferredFrame.wrap(self._expr)
+ elif types in [{int}, {type(None), int}]:
Review comment:
Ah, yes, I meant to go back and change this. Thanks.
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -32,9 +32,74 @@ def __array__(self, dtype=None):
raise frame_base.WontImplementError(
'Conversion to a non-deferred a numpy array.')
+ get = frame_base.not_implemented_method('get')
+
@frame_base.DeferredFrame._register_for(pd.Series)
class DeferredSeries(DeferredDataFrameOrSeries):
+ def __getitem__(self, key):
+ if _is_null_slice(key) or key is Ellipsis:
+ return self
+
+ elif (isinstance(key, int) or _is_integer_slice(key)
+ ) and self._expr.proxy().index._should_fallback_to_positional():
+ raise frame_base.WontImplementError('order sensitive')
+
+ elif isinstance(key, slice) or callable(key):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df: df[key],
+ [self._expr],
+ requires_partition_by=partitionings.Nothing(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif isinstance(key, DeferredSeries):
+ return frame_base.DeferredFrame.wrap(
+ expressions.ComputedExpression(
+ # yapf: disable
+ 'getitem',
+ lambda df, indexer: df[indexer],
+ [self._expr, key._expr],
+ requires_partition_by=partitionings.Index(),
+ preserves_partition_by=partitionings.Singleton()))
+
+ elif pd.core.series.is_iterator(key) or pd.core.common.is_bool_indexer(key):
+ raise frame_base.WontImplementError('order sensitive')
+
+ else:
+ # We could consider returning a deferred scalar, but that might
+ # be more surprising than a clear error.
+ raise frame_base.WontImplementError('non-deferred')
+
+ if isinstance(key, frame_base.DeferredBase):
+ # Fail early if key is a DeferredBase as it interacts surprisingly with
+ # key in self._expr.proxy().columns
+ raise NotImplementedError(
+ "Indexing with a deferred frame is not yet supported. Consider "
+ "using df.loc[...]")
+
+ if isinstance(key, slice):
+ types = set([type(key.start), type(key.stop), type(key.step)])
+ if types == {type(None)}:
+ # Empty slice is just a copy.
+ return frame_base.DeferredFrame.wrap(self._expr)
+ elif types in [{int}, {type(None), int}]:
+ # This depends on the contents of the index.
+ raise frame_base.WontImplementError(
+ 'Use iloc or loc with integer slices.')
Review comment:
Eventually we may make iloc work for integer indices, but if not they'll get a better error there. The problem with directing users to loc directly is that `df.loc[ix]` is not a drop in replacement for `df[ix]` here, in fact it can be quite different, and so we need to force people to think about what they're trying to do.
##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -453,17 +518,31 @@ def __getattr__(self, name):
def __getitem__(self, key):
# TODO: Replicate pd.DataFrame.__getitem__ logic
- if isinstance(key, frame_base.DeferredBase):
+ if isinstance(key, DeferredSeries) and key._expr.proxy().dtype == bool:
Review comment:
Ah, yes, done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13122:
URL: https://github.com/apache/beam/pull/13122#issuecomment-708757883
R: @robinyqiu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #13122:
URL: https://github.com/apache/beam/pull/13122#issuecomment-709570433
@TheNeuralBit Could you help with the review? I am currently busy with the release.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13122: [BEAM-9547] More complete indexing capabilities.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13122:
URL: https://github.com/apache/beam/pull/13122#issuecomment-712347157
Yes, I was running against the user guide, but I've now added some explicit tests to our own doctests 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org