Skip to content

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Feb 25, 2025

Which issue does this PR close?

Rationale for this change

The regeneration logic uses a fork of the sqllogictest runner. However, the runner has been updated on main and the fork hasn't been updated.

This means we can't update the sqllogictest tests anymore , and we hit this when working with @jayzhan211 on #14824 (comment)

What changes are included in this PR?

  1. Add two other files that need the old version
  2. Update the updates for changes on main
  3. Also add a note in the docs about the regenerate_sqlite_files.sh script

Are these changes tested?

I tested them manually

Are there any user-facing changes?

No, this is a development only change

However, I think we need to come up with a more maintainable solution for maintaining these files

I'll file a ticket describing that need later

@alamb alamb added the development-process Related to development process of DataFusion label Feb 25, 2025
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed development-process Related to development process of DataFusion labels Feb 25, 2025
@@ -0,0 +1,131 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is pretty brutal -- I edited these files so they compiled and then copied them to the regenerate location, similarly to how it is tone for the runner.rs

cp datafusion/sqllogictest/regenerate/sqllogictests.rs datafusion/sqllogictest/bin/sqllogictests.rs
# replace the sqllogictest.rs with a customized versions.
cp datafusion/sqllogictest/regenerate/sqllogictests.rs datafusion/sqllogictest/bin/sqllogictests.rs
cp datafusion/sqllogictest/regenerate/src/engines/postgres_engine/ datafusion/sqllogictest/src/engines/postgres_engine/mod.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

two other files are now copied here...

@alamb
Copy link
Contributor Author

alamb commented Feb 25, 2025

I am still testing this locally

@alamb alamb force-pushed the alamb/fix_update_sqllogic_test branch from bacbdfa to b4bbb2b Compare February 25, 2025 22:11
@alamb
Copy link
Contributor Author

alamb commented Feb 25, 2025

Ok, I think this one now works again.

@alamb alamb marked this pull request as ready for review February 25, 2025 22:24
@alamb
Copy link
Contributor Author

alamb commented Feb 25, 2025

@Omega359 has an alternate plan here potentially: #14824 (comment)

@jayzhan211
Copy link
Contributor

Thanks @alamb, let me try it

@jayzhan211 jayzhan211 merged commit 3193014 into apache:main Feb 26, 2025
24 checks passed

```shell
PG_URI=postgresql://postgres@localhost:5432/postgres bash datafusion/sqllogictest/regenerate_sqlite_files.sh
```
Copy link
Contributor

@jayzhan211 jayzhan211 Feb 26, 2025

Choose a reason for hiding this comment

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

postgresql://postgres@localhost:5432/postgres doesn't work for me.

Run with $(whoami) to find your name, and the command should be

PG_URI=postgres://$(whoami)@localhost:5432/{database} bash datafusion/sqllogictest/regenerate_sqlite_files.sh

And run this to find database

➜  datafusion git:(count-schema-name) ✗ psql -U jayzhan -h localhost -p 5432 -l
                           List of databases
   Name    |  Owner  | Encoding | Collate | Ctype |  Access privileges  
-----------+---------+----------+---------+-------+---------------------
 postgres  | jayzhan | UTF8     | C       | C     | 
 template0 | jayzhan | UTF8     | C       | C     | =c/jayzhan         +
           |         |          |         |       | jayzhan=CTc/jayzhan
 template1 | jayzhan | UTF8     | C       | C     | =c/jayzhan         +
           |         |          |         |       | jayzhan=CTc/jayzhan
(3 rows)

This works

PG_URI=postgres://jayzhan@localhost:5432/postgres bash datafusion/sqllogictest/regenerate_sqlite_files.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

I just run postgres in docker:

docker run --name df_postgres -e POSTGRES_USER=test -e POSTGRES_PASSWORD=test -e POSTGRES_DB=test -d -p 5432:5432 postgres:latest
export PG_URI=postgres://test:test@host.docker.internal:5432/test
./datafusion/sqllogictest/regenerate_sqlite_files.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants