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.

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

Lift update binding logic from DuckTableEntry to TableCatalogEntry #7874

Merged
merged 109 commits into from
Jun 13, 2023
Merged

Lift update binding logic from DuckTableEntry to TableCatalogEntry #7874

merged 109 commits into from
Jun 13, 2023

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Jun 8, 2023

Binding the update operator requires some careful handling of indices. All of that currently happens in the DuckTableEntry class but it seems there's nothing really DuckTable specific in it. We can lift it to the base class TableCatalogEntry, so the logic is available for custom catalog implementation.

@Mytherin
Copy link
Collaborator

Mytherin commented Jun 8, 2023

Thanks for the PR! LGTM. This might require some tweaking in the SQLite extension as well. The single-file compilation (compiling with DISABLE_UNITY=1) is also failing in the CI - likely due to some missing includes. Could you have a look at that?

@bleskes
Copy link
Contributor Author

bleskes commented Jun 9, 2023

@Mytherin the compilation issue should be fixed now. Any tips on how to the SQLite extension? I'm not sure what the process is because in another repo

@Mytherin
Copy link
Collaborator

Mytherin commented Jun 9, 2023 •
edited

Generally we send a PR to the external repo and merge it first, and then update extensions.csv with the new git hash as part of the PR here

bleskes added a commit to bleskes/sqlite_scanner that referenced this pull request Jun 9, 2023
@bleskes
companion to duckdb/duckdb/pull/7874
82b17db
@bleskes bleskes mentioned this pull request Jun 9, 2023
Merged
@bleskes
Copy link
Contributor Author

bleskes commented Jun 9, 2023

@Mytherin I've put up a pr to sql lite, but I'm clearly not following the right process to validate it. Can you point me at where I can read the steps?

Mytherin added a commit to duckdb/sqlite_scanner that referenced this pull request Jun 9, 2023
@Mytherin
Merge pull request #52 from bleskes/update_static_bind_methods
05dd50b 
Companion to duckdb/duckdb/pull/7874
@Mytherin
Copy link
Collaborator

Mytherin commented Jun 9, 2023

The SQLite scanner still uses the old extension mechanism which is not very well documented. You can update extensions.csv in this PR (and run the format fix) in order to move the SQLite version forward.

@bleskes
Copy link
Contributor Author

bleskes commented Jun 9, 2023

@Mytherin done. Thanks.

@bleskes
Copy link
Contributor Author

bleskes commented Jun 12, 2023

@Mytherin lmk if the thread santinzer issue looks to you like it was caused by this pr.

@Mytherin
Copy link
Collaborator

The thread sanitizer issues appear to be caused by GetStorageInfo not grabbing a lock correctly - which now triggers the issue because the function is used in more places. I have (hopefully) pushed a fix for this.

Tishj and others added 14 commits June 12, 2023 15:33
@Tishj @bleskes
apply feedback, partially
61b4c9a
@Mytherin @bleskes
Disable attaching on-disk DuckDB databases if external access is disa…
269de27 
…bled
@Mytherin @bleskes
Fix duckdb#7711 - disallow detaching the currently USEd database
aac9821
@Tishj @bleskes
only execute in Close if it was never executed before
e45e6eb
@hannes @bleskes
add duckdb:::rel_from_table_function for R
69217a1
@hannes @bleskes
basic test
85bf409
@hannes @bleskes
mc kirill's suggestions
3d70e4a
@hannes @bleskes
trying to disable hive partitioning on R relational test case to not …
765022f 
…trigger UBSAN on RE2
@hannes @bleskes
removed offending test case entirely
7d06ffa
@Tishj @bleskes
achieve the same behavior, but return None from the __exit__ method i…
7703869 
…nstead, fixes duckdb#7816
@Mytherin @bleskes
Reservoir sample - when appending a chunk to two different reservoirs…
4efabac 
… we need to explicitly slice it
@Mytherin @bleskes
Correctly check for nullpointer in SQLite shell function
b22efa9
@Mytherin @bleskes
Correctly deal with extreme numbers in round precision
9a77a84
@Mytherin @bleskes
Fix negation underflow in list extract
1a4bdf4
pdet and others added 24 commits June 12, 2023 15:34
@pdet @bleskes
Test reset fix
4a19fb6
@pdet @bleskes
Ensure the option works on python
1c53199
@pdet @bleskes
R
1b7edfe
@pdet @bleskes
Bring timezone to arrowoptions
5b25698
@pdet @bleskes
Tidy fixes
82fbbaa
@pdet @bleskes
Thijs' feeback
804e64d
@pdet @bleskes
Remove IDE include
acf6d66
@pdet @bleskes
Update arrow oot extension
cafe624
@pdet @bleskes
I changed the wrong tag
e06ade8
@pdet @bleskes
Update arrow tag
9cb9df5
@pdet @bleskes
New wasm tag
5dd86a4
@pdet @bleskes
This issue seems unrelated to this PR, but adding a lock to verify me…
f665d6e 
…thod of parallel reader
@pdet @bleskes
Update uncovered files for CodeCov CI
58bb36f
@pdet @bleskes
somehow i renamed this whole thing by accident
595c985
@hannes @bleskes
fix for table in function binding
69dbf07
@hannes @bleskes
fixing python map
eb313bd
@Flogex @bleskes
Implement DatabaseManager::SetDefaultDatabase
be7ca69 
The `DatabaseManager::default_database` is returned by `DatabaseManager::GetDefaultDatabase`
when the default catalog in the search path is invalid. This commit adds a method to change
`default_database`.
This makes it possible that new `Connection`s can use a database other than 'memory' as default.
@Flogex @bleskes
Use 'ClientContext::RunFunctionInTransaction' in 'SetDefaultDatabase'
bae9943
@Flogex @bleskes
Removed call to 'RunFunctionInTransaction' in 'DatabaseManager::SetDe…
2c72c31 
…faultDatabase'

Most functions that call 'SetDefaultDatabase' will already have an active transaction,
hence it is not necessary to create a new one.
@Mytherin @bleskes
Update database_manager.cpp
86d80b4
@Mytherin @bleskes
Fix duckdb#7902 - add support for blob to approx count distinct
db5eae7
@Mytherin @bleskes
Test fixes + codecov ignores
0bd0763
@bleskes
feebdack
4f94680
@bleskes
Merge remote-tracking branch 'duckdb_labs/master' into update_static_…
8756909 
…bind_methods
carlopi
carlopi reviewed Jun 12, 2023
View reviewed changes
src/include/duckdb/catalog/catalog_entry/table_catalog_entry.hpp Outdated
@@ -37,7 +37,7 @@ class TableFunction;
struct FunctionData;

class TableColumnInfo;
class TableIndexInfo;
class ColumnSegmentInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class ColumnSegmentInfo;
struct ColumnSegmentInfo;

carlopi
carlopi reviewed Jun 12, 2023
View reviewed changes
Copy link
Contributor

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

  • I would add line:
catalog/catalog_entry/table_catalog_entry.cpp 3

somewhere in .github/config/uncovered_lines.csv.

Thanks!

src/include/duckdb/storage/table/row_group.hpp Outdated Show resolved Hide resolved
src/include/duckdb/storage/table/row_group_collection.hpp Outdated Show resolved Hide resolved
Mytherin and others added 3 commits June 12, 2023 22:56
@Mytherin @carlopi
Apply suggestions from code review
7a694ae 
Co-authored-by: Carlo Piovesan <piovesan.carlo@gmail.com>
@Mytherin
Add uncovered file
1ebc17a
@bleskes
one more struct
a19abe8
@Mytherin Mytherin merged commit 4b9c7fb into duckdb:master Jun 13, 2023
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlopi carlopi carlopi left review comments

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

Successfully merging this pull request may close these issues.

None yet