Skip to content

Conversation

@Suor
Copy link
Contributor

@Suor Suor commented Nov 13, 2019

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does data depend on whether or not builtin_str is a str or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function converts all builtin_str to str (imported from utils.compat) in a nested structure, which is a noop if they are the same, basically in Python 3.

@efiop
Copy link
Contributor

efiop commented Nov 13, 2019

@Suor Thanks, @Suor ! Mind elaborating on this and showing how much this saves us?

@Suor
Copy link
Contributor Author

Suor commented Nov 14, 2019

@efiop I haven't benched it, don't want to waste time benching this since it will be dropped with Python 2 anyway in near future. This change makes it's even more obvious that we should drop it.

I came across this while optimizing schema validation, the second slowest thing in stage collection.

@efiop
Copy link
Contributor

efiop commented Nov 15, 2019

@Suor if this is python2 dependent, then put it into compat or something to make it more obvious.

@Suor Suor force-pushed the convert-to-unicode branch from 00fa18a to 3943b0e Compare November 15, 2019 15:31
@Suor
Copy link
Contributor Author

Suor commented Nov 15, 2019

Moved it to compat.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thanks!

@efiop efiop merged commit 276621b into treeverse:master Nov 16, 2019
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.

2 participants