Skip to content

Update readers.py in cases where the next record contains Empty list instead of None #275

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

Merged
merged 3 commits into from
Apr 21, 2025

Conversation

rrmina
Copy link
Contributor

@rrmina rrmina commented Apr 21, 2025

Handle cases in readers.py where the next record is an empty list instead of None

Otherwise, it will throw a ValueError of

ValueError: The values set to records are against the schema, expect len 1, got len 0

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2025

CLA assistant check
All committers have signed the CLA.

odps/readers.py Outdated
@@ -360,6 +360,8 @@ def __next__(self):
values = self._readline()
if values is None:
raise StopIteration
if len(values) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to merge this condition with the previous one?

Copy link
Contributor Author

@rrmina rrmina Apr 21, 2025

Choose a reason for hiding this comment

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

You mean like?

if values is None or len(values) == 0:
    raise StopIteration

the len part will throw a TypeError for objects that are truly NoneType

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not happen if values is None as Python will not evaluate the second expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right @wjsi . Sorry for my mistake. Indeed it is possible to merge the condition to the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to reflect the changes in this discussion.

@wjsi wjsi merged commit cef7539 into aliyun:master Apr 21, 2025
1 check passed
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.

3 participants