Skip to content
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

Implements delete() for Index #1165

Merged
merged 15 commits into from May 26, 2020
Merged

Implements delete() for Index #1165

merged 15 commits into from May 26, 2020

Conversation

itholic
Copy link
Contributor

@itholic itholic commented Jan 2, 2020

https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Index.delete.html#pandas.Index.delete

>>> kidx = ks.Index([10, 10, 9, 8, 4, 2, 4, 4, 2, 2, 10, 10])
>>> kidx
Int64Index([10, 10, 9, 8, 4, 2, 4, 4, 2, 2, 10, 10], dtype='int64')

>>> kidx.delete(0)
Int64Index([10, 9, 8, 4, 2, 4, 4, 2, 2, 10, 10], dtype='int64')

>>> kidx.delete([0, 1, 2, 3, 10, 11])
Int64Index([4, 2, 4, 4, 2, 2], dtype='int64')

@itholic
Copy link
Contributor Author

itholic commented Jan 2, 2020

Now we have some problem with MultiIndex related with adding default index like below.

>>> sdf.show()
+---------------+
|__index_value__|
+---------------+
|         [a, x]|
|         [b, y]|
+---------------+
>>> sdf = _InternalFrame.attach_default_index(
...             sdf, default_index_type='distributed-sequence')
Traceback (most recent call last):
...
    raise TypeError("Unsupported type in conversion to Arrow: " + str(dt))
TypeError: Unsupported type in conversion to Arrow: StructType(List(StructField(__index_level_0__,StringType,false),StructField(__index_level_1__,StringType,false)))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
...
NotImplementedError: Invalid returnType with grouped map Pandas UDFs: StructType(List(StructField(__index_level_0__,LongType,true),StructField(__index_value__,StructType(List(StructField(__index_level_0__,StringType,false),StructField(__index_level_1__,StringType,false))),false))) is not supported

so explicitly doesn't support for now.

@codecov-io
Copy link

codecov-io commented Jan 2, 2020

Codecov Report

Merging #1165 into master will increase coverage by 2.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
+ Coverage   92.83%   95.02%   +2.18%     
==========================================
  Files          34       34              
  Lines        7944     7961      +17     
==========================================
+ Hits         7375     7565     +190     
+ Misses        569      396     -173     
Impacted Files Coverage Δ
databricks/koalas/missing/indexes.py 100.00% <ø> (ø)
databricks/koalas/indexes.py 97.16% <100.00%> (+0.09%) ⬆️
databricks/koalas/plot.py 94.28% <0.00%> (+0.95%) ⬆️
databricks/koalas/namespace.py 86.57% <0.00%> (+1.57%) ⬆️
databricks/koalas/frame.py 96.61% <0.00%> (+3.17%) ⬆️
databricks/conftest.py 96.22% <0.00%> (+7.54%) ⬆️
databricks/koalas/__init__.py 93.61% <0.00%> (+14.89%) ⬆️
databricks/koalas/usage_logging/usage_logger.py 100.00% <0.00%> (+50.00%) ⬆️
databricks/koalas/usage_logging/__init__.py 94.59% <0.00%> (+70.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fab9f6f...cb64d1e. Read the comment docs.

@HyukjinKwon
Copy link
Member

Seems okay otherwise.

@HyukjinKwon
Copy link
Member

@itholic, can you resolve conflicts?

@itholic
Copy link
Contributor Author

itholic commented Jan 9, 2020

@HyukjinKwon i did :)

@softagram-bot
Copy link

Softagram Impact Report for pull/1165 (head commit: 8f78dc8)

⚠️ Copy paste found

ℹ️ test_indexes.py: Copy paste fragment on line 32 shared with ../test_numpy_compat.py:

    def pdf(self):
        return pd.DataFrame({
            'a': [1, 2, 3, 4, 5, 6, 7, 8, 9],
            'b': [4, 5, 6, 3, 2, 1, 0, 0, 0],
        }, index=[0, 1, 3, 5, 6, 8, 9, 9, 9])...(truncated 101 chars)

ℹ️ indexes.py: Copy paste fragment inside the same file on lines 787, 1496:

            raise NotImplementedError(
                \"Doesn't support symmetric_difference between Index & MultiIndex for now\")

        sdf_self = self._kdf._s...(truncated 477 chars)

Now that you are on the file, it would be easier to pay back some tech. debt.

⭐ Change Overview

Showing the changed files, dependency changes and the impact - click for full size
(Open in Softagram Desktop for full details)

📄 Full report

Impact Report explained. Give feedback on this report to support@softagram.com

databricks/koalas/indexes.py Outdated Show resolved Hide resolved
databricks/koalas/indexes.py Outdated Show resolved Hide resolved
databricks/koalas/indexes.py Outdated Show resolved Hide resolved
databricks/koalas/indexes.py Outdated Show resolved Hide resolved
@itholic
Copy link
Contributor Author

itholic commented Mar 16, 2020

@HyukjinKwon Yeah, I'm fixing those things you commented. Thanks for the remind :)

@itholic itholic closed this Mar 27, 2020
@itholic itholic deleted the i_delete branch March 27, 2020 02:21
@itholic itholic reopened this Mar 27, 2020
@HyukjinKwon
Copy link
Member

@itholic looks you made a mistake during rebasing the branch ..

@itholic
Copy link
Contributor Author

itholic commented Apr 8, 2020

@HyukjinKwon Right. I'm fixing now 😭

@itholic
Copy link
Contributor Author

itholic commented Apr 8, 2020

I fixed but It seems that there is some problem in CI with conda.

@ueshin
Copy link
Collaborator

ueshin commented Apr 8, 2020

I'm taking a look at the conda build.

@ueshin ueshin closed this Apr 8, 2020
@ueshin ueshin reopened this Apr 8, 2020
@ueshin
Copy link
Collaborator

ueshin commented Apr 8, 2020

@itholic The build must be fixed, maybe we should push another commit to cleanup the checks.

@itholic
Copy link
Contributor Author

itholic commented Apr 8, 2020

@ueshin

Thanks for the notice! I just pushed an empty commit :)

@ueshin
Copy link
Collaborator

ueshin commented Apr 8, 2020

I'm wondering how to restart the checks cleanly. haha

@itholic
Copy link
Contributor Author

itholic commented Apr 8, 2020

@ueshin

Oh, It seems that re-run the Actions is allowed for the repository owner in the Actions page.

스크린샷 2020-04-08 오후 12 04 45

(the screenshot was captured in my forked repository)

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2020

Codecov Report

Merging #1165 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
+ Coverage   94.17%   94.18%   +0.01%     
==========================================
  Files          38       38              
  Lines        8553     8570      +17     
==========================================
+ Hits         8055     8072      +17     
  Misses        498      498              
Impacted Files Coverage Δ
databricks/koalas/missing/indexes.py 100.00% <ø> (ø)
databricks/koalas/indexes.py 97.13% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04c7bab...af9f0a4. Read the comment docs.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

@itholic
Copy link
Contributor Author

itholic commented May 25, 2020

@HyukjinKwon Thanks!

@HyukjinKwon
Copy link
Member

@itholic can you rebase this please?

@itholic
Copy link
Contributor Author

itholic commented May 26, 2020

@HyukjinKwon Yup, pending tests.

@HyukjinKwon HyukjinKwon merged commit 35bcf7f into databricks:master May 26, 2020
@itholic itholic deleted the i_delete branch May 29, 2020 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants