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

Bump DuckDB, properly propagate wasm_* platform and add test #1328

Merged
merged 10 commits into from
Jul 18, 2023

Conversation

carlopi
Copy link
Collaborator

@carlopi carlopi commented Jul 17, 2023

Previously the custom provided platform was not propagated in the right place, fixed and added a test.

Fixes #1298

@Mytherin
Copy link
Contributor

Thanks for the PR! Are all the console.log messages left in intentionally?

@carlopi
Copy link
Collaborator Author

carlopi commented Jul 18, 2023

Yes, it's not great practice, but currently strategy of shallowing exceptions is not great since shallows both DuckDB-side exceptions (like failures) and JS exceptions (these might be harmless, and might make sense to skip in some cases).

To users not opening the console, it should give roughly the same experience, but upon inspection some information of what went wrong is available.
Unsure whether to consider this a blocker to tag a version or not, but we can check tomorrow.

Note that exceptions are still used only for exceptional behaviour, so I would assume (but I am not completely sure) that the number of exceptions will be low (bounded by the number of failed queries ?).

@Mytherin Mytherin merged commit 5741146 into duckdb:master Jul 18, 2023
11 checks passed
@Mytherin
Copy link
Contributor

Alright, makes sense to me. Thanks

@whscullin
Copy link
Contributor

whscullin commented Dec 5, 2023

Hi, just came across this when diagnosing log spamming during some of our tests. It would be nice to be able to disable the currently unconditional console.log so we can have clean test outputs. Working around this mocking console.log for now. It looks like it's difficult (impossible) to mock this away because it's in the worker.

@whscullin
Copy link
Contributor

I guess it's unclear to me why this isn't just using the Logger interface, at least in the worker where it's available. that seems like it would allow for configuration. I can do a PR for that if that seems reasonable.

@carlopi
Copy link
Collaborator Author

carlopi commented Dec 6, 2023

I moved this to an independent issue, #1518.

Why this happened is an oversight on my side, there was a comment like:

                    // Next few lines should argubly in separate JavaScript-land function call
                    // TODO: move them out / have them configurable

that I never got to address.

Fix is trivial, only problem is that this is in DuckDB codebase and will require patching while building duckdb in duckdb-wasm, but there is already infrastructure for that.

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.

Error sort query with array of struct type
3 participants