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

Fix l_quantity data generated by Tpch connector #11511

Closed

Conversation

pramodsatya
Copy link
Collaborator

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 12, 2024
Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9953853
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/673355984589ad0008f24ee1

@aditi-pandit
Copy link
Collaborator

@pramodsatya : Why is this fix only for l_quantity ? I see that other columns like lineitem.discount are also fixed with decimalToDouble. Do we not see inconsistencies with those columns ?

@pramodsatya
Copy link
Collaborator Author

pramodsatya commented Nov 12, 2024

@pramodsatya : Why is this fix only for l_quantity ? I see that other columns like lineitem.discount are also fixed with decimalToDouble. Do we not see inconsistencies with those columns ?

@aditi-pandit, other double columns like lineitem.discount do not have an inconsistency for SF1 and they need the function decimalToDouble.
For tiny SF, the double column l_extendedprice and identifier columns l_partkey, l_suppkey have a data mismatch, in addition to the Varchar columns containing comments (noted here). Since for all these columns the inconsistency originates from Duckdb's dbgen, I wanted to take it up in a separate PR. Would that be fine?

Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @pramodsatya

@facebook-github-bot
Copy link
Contributor

@amitkdutta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Thanks @pramodsatya for fixing it so quickly.

@facebook-github-bot
Copy link
Contributor

@amitkdutta merged this pull request in f1ff2df.

Copy link

Conbench analyzed the 1 benchmark run on commit f1ff2dff.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@pramodsatya pramodsatya deleted the tpch_fix_datagen branch November 14, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TPCH connector return different results on prestissimo for l_quantity
6 participants