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

ENH/BUG: Support string slicing with other expressions #1627

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@cpcloud
Copy link
Member

cpcloud commented Sep 17, 2018

Closes #1626

@cpcloud cpcloud added this to the Next Release milestone Sep 17, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-str-slice branch from 801b858 to 054f5b0 Sep 19, 2018

@cpcloud

This comment has been minimized.

Copy link
Member Author

cpcloud commented Sep 19, 2018

@kszucs Can you give this a review when you get a chance?

@cpcloud cpcloud requested a review from kszucs Sep 19, 2018

raise ValueError('negative slicing not yet supported')
if not isinstance(stop, ir.Expr):
if stop is not None and stop < 0:
raise ValueError('negative slicing not yet supported')

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

Uppercase n

if stop is not None and stop < 0:
raise ValueError('negative slicing not yet supported')
if stop is None:
stop = self.length()

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

Minor, just not consistent with the previous validation order.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Sep 19, 2018

Author Member

I don't follow, what was the previous validation order?

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

is None goes to the top + elif

This comment has been minimized.

Copy link
@cpcloud

cpcloud Sep 19, 2018

Author Member

Oh, nevermind, I see it

Show resolved Hide resolved ibis/expr/api.py
def execute_string_substring_series_series(op, data, start, length, **kwargs):
end = start + length

def iterate(value,

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

This "general" case wouldn't be enough?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Sep 19, 2018

Author Member

I suppose we could farm out the other functions to this one.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Sep 19, 2018

Author Member

We definitely need to handle the integer, series and series, integer cases distinctly, but those two can be made to be instances of the general case.

if start is None:
start = 0
if start < 0:
raise ValueError('Negative slicing not yet supported')

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

Perhaps self.length() + start?

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 19, 2018

Member

Crossed out because of this comment.

@cpcloud cpcloud force-pushed the cpcloud:fix-str-slice branch from b562d92 to c8a8867 Sep 19, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-str-slice branch from c8a8867 to 2494069 Sep 19, 2018

Show resolved Hide resolved ibis/expr/api.py
@kszucs

kszucs approved these changes Sep 19, 2018

@cpcloud

This comment has been minimized.

Copy link
Member Author

cpcloud commented Sep 19, 2018

Merging on green

@cpcloud cpcloud closed this in fbe516c Sep 19, 2018

@cpcloud cpcloud deleted the cpcloud:fix-str-slice branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.