Skip to content

Conversation

@dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 28, 2025

I see people complain on crash #652.

Checking the source code,

result->timestamp_filter = "NOW() - INTERVAL '" + older_than_default + "'";

ducklake is directly using NOW() - INTERVAL <interval> syntax, which is not supported by sqlite
This PR converts deadline to ISO-8601 compatible string, which is acceptable for all existing databases (i.e., duckdb, sqlite, postgres).

How I tested

memory D LOAD sqlite_scanner;
memory D ATTACH DATABASE 'sqlite:test_metadata.db' AS testcat (TYPE DUCKLAKE, DATA_PATH '/tmp/issue_652/');
memory D INSERT INTO testcat.test_table VALUES (1, 'test'), (2, 'data');
memory D CHECKPOINT testcat;
┌─────────┐
│ Success │
│ boolean │
├─────────┤
│ 0 rows  │
└─────────┘

@dentiny dentiny force-pushed the hjiang/fix-timestamp-dilect branch from fbfe55b to 0ece568 Compare December 28, 2025 06:26
@dentiny
Copy link
Contributor Author

dentiny commented Dec 29, 2025

Failed CI tests don't seem to related to my change:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
unittest is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
test/sql/default/default_expressions.test
-------------------------------------------------------------------------------
/duckdb_build_dir/duckdb/test/sqlite/test_sqllogictest.cpp:261
...............................................................................

test/sql/default/default_expressions.test:84: FAILED:
explicitly with message:
  0

1. test/sql/default/default_expressions.test:84
================================================================================
Query unexpectedly succeeded! (test/sql/default/default_expressions.test:84)!
================================================================================
ALTER TABLE t_2 ADD COLUMN j INTEGER DEFAULT RANDOM();
================================================================================
Success	
BOOLEAN	
[ Rows: 0]

2. test/sql/concurrent/file_level_conflict.test:34
================================================================================
Query unexpectedly failed! (test/sql/concurrent/file_level_conflict.test:34)!
================================================================================
DELETE FROM ducklake.tbl WHERE key = 1;
================================================================================
TransactionContext Error: Failed to commit: Failed to commit DuckLake transaction.
Calling GetValueInternal on a value that is NULL
[146/257] (56%): test/sql/concurrent/file_level_conflict.test-------------------------------------------------------------------------------
test/sql/concurrent/file_level_conflict.test
-------------------------------------------------------------------------------
/duckdb_build_dir/duckdb/test/sqlite/test_sqllogictest.cpp:261
...............................................................................

test/sql/concurrent/file_level_conflict.test:34: FAILED:
explicitly with message:
  0

@guillesd guillesd requested a review from pdet December 29, 2025 10:26
@pdet
Copy link
Collaborator

pdet commented Dec 30, 2025

Hey @dentity thanks for the PR. The main branch should be fixed now, can you merge with it?

Also, can you remove the relevant sqlite tests being skipped from the CI tests?

https://github.com/duckdb/ducklake/blob/main/test/configs/sqlite.json#L87

@pdet
Copy link
Collaborator

pdet commented Dec 30, 2025

Instead of merging with latest main this could also go to V1.4

@dentiny dentiny changed the base branch from main to v1.4-andium December 30, 2025 07:58
@dentiny dentiny changed the base branch from v1.4-andium to main December 30, 2025 08:01
@dentiny
Copy link
Contributor Author

dentiny commented Dec 30, 2025

Tried to re-target against v1.4 branch, but found there's conflicts, just want to confirm I should include that as well?

[~/ducklake] (hjiang/fix-timestamp-dilect) 
vscode@9a5948bd4884$ git reset --hard v1.4-andium
HEAD is now at f50618b48c Fix #19455: correctly extract root table in merge into when running ajoin that contains single-sided predicates that are transformed into filters (#19637)

[~/ducklake] (hjiang/fix-timestamp-dilect) 
vscode@9a5948bd4884$ git cherry-pick 0ece568ca90c59b6e34be0d5ec63ee66ce655a98
CONFLICT (modify/delete): src/functions/ducklake_cleanup_files.cpp deleted in HEAD and modified in 0ece568ca9 (Attempt to fix time dilect).  Version 0ece568ca9 (Attempt to fix time dilect) of src/functions/ducklake_cleanup_files.cpp left in tree.
error: could not apply 0ece568ca9... Attempt to fix time dilect
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"

@pdet
Copy link
Collaborator

pdet commented Dec 31, 2025

I'm a bit confused on how you are getting conflicts from your commit, since both src/functions/ducklake_cleanup_files.cpp and test/configs/sqlite.json seem to have no changes.

@dentiny dentiny force-pushed the hjiang/fix-timestamp-dilect branch from e31ba5e to d588ef6 Compare December 31, 2025 12:08
@dentiny dentiny changed the base branch from main to v1.4-andium December 31, 2025 12:08
@dentiny
Copy link
Contributor Author

dentiny commented Dec 31, 2025

I'm a bit confused on how you are getting conflicts from your commit

I just realized my own fork v1.4 branch didn't sync upstream, should be good now

@dentiny
Copy link
Contributor Author

dentiny commented Dec 31, 2025

Hi Pedro, I think I've rebased upon v1.4 branch, with two fixing commits in this PR.
But CI still complains on the old issue, which you mentioned we've already have it fixed?

1. test/sql/concurrent/file_level_conflict.test:34
================================================================================
Query unexpectedly failed! (test/sql/concurrent/file_level_conflict.test:34)!
================================================================================
DELETE FROM ducklake.tbl WHERE key = 17;
================================================================================
TransactionContext Error: Failed to commit: Failed to commit DuckLake transaction.
Calling GetValueInternal on a value that is NULL
[148/262] (56%): test/sql/concurrent/file_level_conflict.test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
unittest is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
test/sql/concurrent/file_level_conflict.test
-------------------------------------------------------------------------------
/duckdb_build_dir/duckdb/test/sqlite/test_sqllogictest.cpp:261
...............................................................................

test/sql/concurrent/file_level_conflict.test:34: FAILED:
explicitly with message:
  0

@pdet
Copy link
Collaborator

pdet commented Dec 31, 2025

It seems that the CI is running on main, and this is an issue recently introduced in duckdb-main.

cd duckdb && git checkout main
Warning: you are leaving 1 commit behind, not connected to
any of your branches:

  5d5d04f418 Remove enable_verifications

If you want to keep it by creating a new branch, this may be a good time
to do so with:

 git branch <new-branch-name> 5d5d04f418

Switched to branch 'main'
Your branch is up to date with 'origin/main'.

Let me try to restart your CI.

@pdet pdet marked this pull request as draft December 31, 2025 13:53
@pdet pdet marked this pull request as ready for review December 31, 2025 13:53
@pdet
Copy link
Collaborator

pdet commented Jan 2, 2026

Thanks!

@pdet pdet merged commit d29fcce into duckdb:v1.4-andium Jan 2, 2026
10 of 12 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.

2 participants