[CALCITE-5529] Improve dialect tests, part 1: check each query against a reference dialect #4206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Require that newlines in Java string literals are at the end of a fragment. This forces developers to break string literals over several lines, to match the contents of the literals, and helps make long SQL queries and results easier to read and maintain. Disallow '.' at the end of a line of Java code. (If a chain of method calls or field references is broken over multiple lines, each '.' should be at the start of or in the middle of a line.)
…t a reference dialect Create class RelToSqlFixture (was inner class RelToSqlConverterTest.Sql). This fixture has a list of dialects for which the test is enabled, and a reference dialect (currently always Calcite). When the test calls 'done()' on the fixture, the fixture checks the query against the reference dialect. In future, it will also execute the query against each enabled dialect. To ensure that each test remembers to call 'done'()', add a 'token pool' to the test framework. If a token has been opened but not closed, the framework prints the call stack of the 'open' call and fails the test. Add enum DialectCode, which defines the dialects (or dialect configurations) that are possible to test. Adding a DialectCode has a cost (e.g. a bunch of new Quidem recording files) so we should not add one without a good reason. After this change, all queries succeed against the reference dialect (Calcite). A few cases are disabled due to bugs; we should fix these bugs and enable the tests. This is just part 1. Part 2 will be to get the tests to succeed against another dialect (probably a local instance of Postgres). Part 3 will be to use a Quidem recording rather than a live Postgres instance. Part 4 will be to enable this framework for other tests (e.g. SqlOperatorTest). Close apache#4206
julianhyde
force-pushed
the
5529-dialect-tests
branch
from
d12029c to
d7e1d02
Compare
|
|
ILuffZhe
reviewed
Feb 20, 2025
| final Properties info = new Properties(); | ||
| // Hive for RLIKE, Postgres for ILIKE, Spark for EXISTS, etc. | ||
| info.put(CalciteConnectionProperty.FUN.name(), | ||
| "standard,postgresql,bigquery,hive,spark"); |
There was a problem hiding this comment.
One small question: why those libraries are chosen?
| info.put(CalciteConnectionProperty.SCHEMA.name(), | ||
| schemaSpec.schemaName); | ||
| info.put(CalciteConnectionProperty.CONFORMANCE.name(), | ||
| SqlConformanceEnum.LENIENT.name()); |
There was a problem hiding this comment.
Why do we use LENIENT rather than DEFAULT?
| return new HiveSqlDialect(HiveSqlDialect.DEFAULT_CONTEXT | ||
| .withDatabaseMajorVersion(majorVersion) | ||
| .withDatabaseMinorVersion(minorVersion) | ||
| .withNullCollation(NullCollation.LOW)); |
There was a problem hiding this comment.
DEFAULT_CONTEXT in HiveSqlDialect already has the NullCollation.LOW.
| return dialect(JETHRO); | ||
| } | ||
|
|
||
| RelToSqlFixture withMssql() { |
There was a problem hiding this comment.
Do we need other withMssql for different versions?
NobiGo
reviewed
Feb 20, 2025
| * Incorrect return type for BigQuery TRUNC functions </a>. | ||
| * cases such as | ||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-5757">[CALCITE-5757] | ||
| * Incorrect return type for BigQuery TRUNC functions</a>. |
ILuffZhe
reviewed
Feb 20, 2025
|
|
||
| /** Throws if not all fixtures have been released. */ | ||
| public void assertEmpty() { | ||
| int size = map.size(); |
There was a problem hiding this comment.
Is Pool able to handle this situation:
- when calling
map.size()we get 0 - before calling
map.isEmpty(), a token is added - so the pool is not empty but with size 0
| ? sql | ||
| : dialect(testConfig.refDialectCode).exec(); | ||
| switch (phase) { | ||
| case PREPARE: |
There was a problem hiding this comment.
What's phase PARSE for and do we need to handle it here?
|
I will test it locally |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See CALCITE-5529.