Skip to content

fix(sql): non-keyed LT JOIN could return equal timestamps#3001

Merged
bluestreak01 merged 3 commits intomasterfrom
fixes/lt-join-non-keys-strictly-lower-ts
Feb 21, 2023
Merged

fix(sql): non-keyed LT JOIN could return equal timestamps#3001
bluestreak01 merged 3 commits intomasterfrom
fixes/lt-join-non-keys-strictly-lower-ts

Conversation

@jerrinot
Copy link
Contributor

@jerrinot jerrinot commented Feb 17, 2023 •
edited
Loading

The right side of a LT JOIN must have strictly smaller timestamps. Returning equal timestamps is a bug.

@jerrinot
fix(sql): Non-keyed LT JOIN could return equal timestamps
e64e871 
The right side of a LT JOIN must have strictly smaller timestamp. Returning equal timestamp is a bug.
@jerrinot jerrinot added Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution labels Feb 17, 2023
@jerrinot jerrinot closed this Feb 17, 2023
@jerrinot
fix bad old test
f083c90 
this is effectively reverting #1321
I believe the behaviour before #1321 was actually correct.

also removed the newly added test which was testing the same.
@jerrinot jerrinot reopened this Feb 17, 2023
@jerrinot
Copy link
Contributor Author

jerrinot commented Feb 17, 2023

This is reverting #1275
I believe the original behaviour before #1275 was correct.
An alternative explanation: My understanding of LT JOIN is wrong.

@jerrinot jerrinot requested a review from eugenels February 17, 2023 17:28
@jerrinot
Copy link
Contributor Author

jerrinot commented Feb 20, 2023

the failure looks like Maven infra issues:

2023-02-17T17:47:29.6645310Z 
2023-02-17T17:47:41.5356250Z [ERROR] Plugin org.apache.maven.plugins:maven-clean-plugin:3.2.0 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.apache.maven.plugins:maven-clean-plugin:jar:3.2.0: Could not transfer artifact org.apache.maven.plugins:maven-clean-plugin:pom:3.2.0 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]

-> restarting CI.

@ideoma
Copy link
Collaborator

ideoma commented Feb 20, 2023

[PR Coverage check]

😍 pass : 1 / 1 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/engine/join/LtJoinNoKeyRecordCursorFactory.java 1 1 100.00%

@jerrinot
Copy link
Contributor Author

jerrinot commented Feb 21, 2023

hm, again, that's odd:

2023-02-20T16:15:19.1806090Z [ERROR] Plugin org.apache.maven.plugins:maven-clean-plugin:3.2.0 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.apache.maven.plugins:maven-clean-plugin:jar:3.2.0: Could not transfer artifact org.apache.maven.plugins:maven-clean-plugin:pom:3.2.0 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]
2023-02-20T16:15:19.1807950Z [ERROR] 
2023-02-20T16:15:19.1827390Z [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
2023-02-20T16:15:19.1829260Z [ERROR] Re-run Maven using the -X switch to enable full debug logging.
2023-02-20T16:15:19.1831400Z [ERROR] 
2023-02-20T16:15:19.1832700Z [ERROR] For more information about the errors and possible solutions, please read the following articles:
2023-02-20T16:15:19.1834280Z [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException
2023-02-20T16:15:19.3001220Z The process '/usr/local/bin/mvn' failed with exit code 1
2023-02-20T16:15:19.3005010Z Could not retrieve code analysis results - Maven run failed.

@bluestreak01 bluestreak01 merged commit 1bbd945 into master Feb 21, 2023
@bluestreak01 bluestreak01 deleted the fixes/lt-join-non-keys-strictly-lower-ts branch February 21, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

@eugenels eugenels Awaiting requested review from eugenels

Assignees

No one assigned

Labels

Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jerrinot @ideoma @puzpuzpuz @bluestreak01