Skip to content

Conversation

@duongcongtoai
Copy link
Contributor

@duongcongtoai duongcongtoai commented Apr 9, 2024

Which issue does this PR close?

Closes #9443.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 9, 2024
@duongcongtoai duongcongtoai changed the title Index sqllogictest explain result Prepend sqllogictest explain result with line number Apr 9, 2024
@duongcongtoai duongcongtoai marked this pull request as draft April 9, 2024 18:43
@my-vegetable-has-exploded
Copy link
Contributor

I think ci maybe happy after add feature avro.
I don't know whether INCLUDE_TPCH=true cargo test --test sqllogictests --feature avro or INCLUDE_TPCH=true cargo test --test sqllogictests --all-features would works?

@duongcongtoai duongcongtoai marked this pull request as ready for review April 13, 2024 13:35
@duongcongtoai
Copy link
Contributor Author

Hi @my-vegetable-has-exploded , could you help me review these small changes, conflict is most likely to happen but i know how to deal with it now ;)

@my-vegetable-has-exploded
Copy link
Contributor

my-vegetable-has-exploded commented Apr 14, 2024

Hi @my-vegetable-has-exploded , could you help me review these small changes, conflict is most likely to happen but i know how to deal with it now ;)

I am not sure the specifics of the problem at hand. For branch conflicts, I think it is okay to merge or rebase it and regenerate the sqllogicaltests.

Thanks for your work. @duongcongtoai

@duongcongtoai
Copy link
Contributor Author

I am not sure the specifics of the problem at hand. For branch conflicts, I think it is okay to merge or rebase it and regenerate the sqllogicaltests.

The current generated output matches main now, i think it is good to review

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @duongcongtoai and @my-vegetable-has-exploded -- this looks awesome.

We may be able to make the diffs even easier to check if we can remove the ---

// `sqllogictest` ignores whitespace differences
//
// See https://github.com/apache/arrow-datafusion/issues/6328
let content = l.trim_start();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can remove the ---- stuff now

Maybe we could remove the --- and see if the test diff look better 🤔

Not in this PR

@duongcongtoai
Copy link
Contributor Author

Hi @alamb look like only you can merge it :D

@alamb
Copy link
Contributor

alamb commented Apr 15, 2024

Thanks again @duongcongtoai and @my-vegetable-has-exploded -- I merged up this PR from main and re-ran CI tests just to be sure we didn't introduce any logical conflicts and it all looks good to me. Thanks agian!

@alamb alamb merged commit 03b314c into apache:main Apr 15, 2024
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.

Format plan with line number.

3 participants