-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Test all examples from library-user-guide & user-guide docs #14544
Test all examples from library-user-guide & user-guide docs #14544
Conversation
…les-documentation
datafusion/core/Cargo.toml
Outdated
@@ -152,6 +154,8 @@ serde_json = { workspace = true } | |||
sysinfo = "0.33.1" | |||
test-utils = { path = "../../test-utils" } | |||
tokio = { workspace = true, features = ["rt-multi-thread", "parking_lot", "fs"] } | |||
dashmap = "6.1.0" | |||
snmalloc-rs = "0.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need snmalloc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment below I don't think we can use snmalloc in the dev dependencies without messing up the other builds -- can you please remove that (and maybe add a note to the example that tried to use it?)
Ah, I see the snmalloc comes from running
I think the issue there is that snmalloc takes over the global allocator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @ugoa -- this is looking really cool
Let's try and get this PR ready for merge -- it is already an amazing improvement. I'll try and fix up the tests and get it in later today. Then maybe we can make additional improvements as follow ons
datafusion/core/Cargo.toml
Outdated
@@ -152,6 +154,8 @@ serde_json = { workspace = true } | |||
sysinfo = "0.33.1" | |||
test-utils = { path = "../../test-utils" } | |||
tokio = { workspace = true, features = ["rt-multi-thread", "parking_lot", "fs"] } | |||
dashmap = "6.1.0" | |||
snmalloc-rs = "0.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment below I don't think we can use snmalloc in the dev dependencies without messing up the other builds -- can you please remove that (and maybe add a note to the example that tried to use it?)
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
import re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to DeepSeek generating this code snippet haha, too powerful
I will push a few commits to fix this |
Making the examples in the doc rendred with
And add the Perhaps you can make a follow on PR? |
I plan to merge this PR in once the tests pass |
Ah I didn't know that, so I can generate the doc by run
Sure I can open another PR to address this, cheers. |
The way I do it is ./dev/update_function_docs.sh
🙏 thank you so much |
Ah, I see now the issue is that we'll try and test all examples: https://github.com/apache/datafusion/actions/runs/13241873169/job/36958947894?pr=14544 So they need to be marked as SQL to avoid CI failures. I'll look into it |
Yeah, with just |
I pushed a fix in e0b360c Let's see if we can get a clean run |
So close... |
My pleasure! I learned a lot during the time as well. |
Hey @alamb , btw we probably need to remove the installation of |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Library User Guide:
User Guide:
Are these changes tested?
Yes
Are there any user-facing changes?
No