8000 [Python] UDFs now produce the correct result when used together with `range` by Tishj · Pull Request #7876 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Python] UDFs now produce the correct result when used together with range #7876

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 9, 2023
Merged

[Python] UDFs now produce the correct result when used together with range #7876

merged 3 commits into from
Jun 9, 2023

Conversation

Tishj
Copy link
Contributor
@Tishj Tishj commented Jun 8, 2023 •
edited
Loading

This PR fixes #7773

The title might be too narrow, but I was struggling to find a good concise explanation of what the issue was.

Because the function was not marked as having side effects, ConstantFolding caused us to only execute the function once.
After that was fixed though, in the c++ wrapper for the python udf execution we turned it into a ConstantVector if all the inputs are constant, which made udfs like this produce the wrong result:

fake = faker.Faker()
def generate_date():
    return fake.date_between()

con.create_function(
    'generate_date', 
    generate_date,
    [],
    'DATE'
)

rel = con.sql("""
select *
FROM (
    SELECT generate_date() AS random_date 
    FROM range(100)
)
""")

@Tishj
Copy link
Contributor Author
Tishj commented Jun 8, 2023

@Mytherin should we add a side_effects: bool keyword arg instead of assuming every UDF will have side effects?

@Mytherin
Copy link
Collaborator
Mytherin commented Jun 8, 2023

@Mytherin should we add a side_effects: bool keyword arg instead of assuming every UDF will have side effects?

That sounds good yes

Mytherin
Mytherin approved these changes Jun 8, 2023
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM

@Mytherin Mytherin merged commit 24b6ca7 into duckdb:master Jun 9, 2023
@Mytherin
Copy link
Collaborator
Mytherin commented Jun 9, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scalar UDFs assume Idempotency?
2 participants
@Tishj @Mytherin
0