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

Delete statement #11

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Delete statement #11

merged 5 commits into from
Jul 15, 2024

Conversation

hmeriann
Copy link
Contributor

@hmeriann hmeriann commented Jul 9, 2024

This PR adds GenerateDelete() to Statement Generator.
This function generates the DELETE_STATEMENT for one of the tables from the context or based on the randomly generated table references.

What's more, this PR removes .github/workflows/_extension_deploy.yml file and updates .github/workflows/MainDistributionPipeline.yml to use the _extension_deploy.yml from the extension-ci-tools instead of a local copy, as @samansmink recommended to do. Also the wasm_mvp;wasm_eh;wasm_threads and windows_amd64_rtools are skipped for now.

@hmeriann
Copy link
Contributor Author

hmeriann commented Jul 9, 2024

@Tmonster could you please review this PR?

Copy link
Collaborator

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

Thanks! just a few comments

@@ -16,15 +16,17 @@ jobs:
name: Build extension binaries
uses: duckdb/extension-ci-tools/.github/workflows/[email protected]
with:
duckdb_version: main
duckdb_version: v1.0.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 this not already done? Or was this done earlier and now you are just reverting it?

Copy link
Contributor Author

@hmeriann hmeriann Jul 10, 2024

Choose a reason for hiding this comment

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

Previously we changed it with @carlopi to main and current changes are came from the @samansmink recommendation to match this file to delta extension's along with removing the _extension_deploy.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing: I think it make sense to have the duckdb submodule match what CI compiles

If sqlsmith should strive to be compatible both with v1.0.0 and main, I would have 2 CI jobs (or 2 sections of the same yml file) compile for both cases.

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 would have 2 CI jobs (or 2 sections of the same yml file) compile for both cases.

Do you mean one for the current stable version of the submodule duckdb and one for some another version? I'm not sure I got it, how should it be changed

# this workflow can be configured to use a custom deploy script.


name: Extension Deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file getting deleted? Can you update the PR description?

src/statement_generator.cpp Outdated Show resolved Hide resolved
src/statement_generator.cpp Outdated Show resolved Hide resolved
@hmeriann hmeriann requested a review from Tmonster July 10, 2024 13:24
@Tmonster
Copy link
Collaborator

Looks like the duckdb submodule is being updated? Is that still necessary? It should point to v1.0.0 already right?

@hmeriann
Copy link
Contributor Author

hmeriann commented Jul 10, 2024

Looks like the duckdb submodule is being updated? Is that still necessary? It should point to v1.0.0 already right?

You are right, threw that commit out

@Tmonster
Copy link
Collaborator

LGTM, can you resolve the merge conflicts?

hmeriann and others added 5 commits July 15, 2024 12:50
fix indentation

added two more jobs to build and deploy main version

Update .github/workflows/MainDistributionPipeline.yml

Co-authored-by: Carlo Piovesan <[email protected]>
Copy link
Collaborator

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

@Tmonster Tmonster merged commit c5305fd into duckdb:main Jul 15, 2024
32 checks 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