[GitHub] [beam] TheNeuralBit commented on a change in pull request #16027: [BEAM-12565] Dataframe compare implementation

[GitBox <gi...@apache.org>]

TheNeuralBit commented on a change in pull request #16027:
URL: https://github.com/apache/beam/pull/16027#discussion_r762270793



##########
File path: sdks/python/apache_beam/dataframe/frames.py
##########
@@ -3441,6 +3460,18 @@ def value_counts(self, subset=None, sort=False, normalize=False,
       else:
         return result
 
+  @frame_base.with_docs_from(pd.DataFrame)
+  @frame_base.args_to_kwargs(pd.DataFrame)
+  @frame_base.populate_defaults(pd.DataFrame)
+  def compare(self, other, **kwargs):

Review comment:
       Hi @roger-mike, looking through the docs for this operation [here](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.compare.html), I think we will need to be restrictive about the arguments we support. As you point out, the default (`align_axis=1`, `keep_shape=False`) will drop columns that are equivalent. Since that makes the shape depend on the input data, we should raise `WontImplementError(reason="non-deferred-columns")` in that case.
   
   We should still be able to support `align_axis=1`, `keep_shape=True` though. And I think we should be able to support `align_axis=0` no matter what the other args are.
   
   Does that make sense?

##########
File path: sdks/python/apache_beam/dataframe/frames_test.py
##########
@@ -1082,6 +1082,50 @@ def test_dt_tz_localize_nonexistent(self):
             'Europe/Warsaw', ambiguous='NaT', nonexistent=pd.Timedelta('1H')),
         s)
 
+  def test_compare_series(self):
+    s1 = pd.Series(["a", "b", "c", "d", "e"])
+    s2 = pd.Series(["a", "a", "c", "b", "e"])
+
+    self._run_test(lambda s1, s2: s1.compare(s2), s1, s2)
+    self._run_test(lambda s1, s2: s1.compare(s2, align_axis=0), s1, s2)
+    self._run_test(lambda s1, s2: s1.compare(s2, keep_shape=True), s1, s2)
+    self._run_test(
+        lambda s1, s2: s1.compare(s2, keep_shape=True, keep_equal=True), s1, s2)
+
+  def test_compare_dataframe(self):

Review comment:
       Yeah I think it's approptiate to keep that test skipped. The problem is that it creates test data by modifying the DataFrame in-place with loc, which we don't support:
   ```
   >>> df2 = df.copy()
   >>> df2.loc[0, 'col1'] = 'c'
   >>> df2.loc[2, 'col3'] = 4.0
   >>> df2
     col1  col2  col3
   0    c   1.0   1.0
   1    a   2.0   2.0
   2    b   3.0   4.0
   3    b   NaN   4.0
   4    a   5.0   5.0
   ```
   
   It's fine to just test `compare` here in `frames_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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org