Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test for wrong table #48

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

test for wrong table #48

wants to merge 5 commits into from

Conversation

dbulahov
Copy link

Signed-off-by: dbulahov [email protected]

@dbulahov
test for wrong table
6389996 
Signed-off-by: dbulahov <[email protected]>
@stbischof
Copy link
Contributor

Good test. Really important to also resolve all nested select Statements

manticore-projects
manticore-projects reviewed Nov 21, 2024
View reviewed changes
src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java Outdated
@@ -34,6 +34,8 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.provider.Arguments;

import static org.junit.jupiter.api.Assertions.assertThrowsExactly;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid those static imports.

manticore-projects
manticore-projects reviewed Nov 21, 2024
View reviewed changes
src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java Outdated
.addTable("a", new JdbcColumn("col1"), new JdbcColumn("col2"), new JdbcColumn("col3"))
.addTable("b", new JdbcColumn("col1"), new JdbcColumn("col2"), new JdbcColumn("col3"));

//should be exception bb table not exist
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not check for a specific exception.

manticore-projects
manticore-projects requested changes Nov 21, 2024
View reviewed changes
Copy link
Collaborator

@manticore-projects manticore-projects left a comment

Choose a reason for hiding this comment

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

Please fix format exceptions.
Please check for the specific exception which returns "BB", not just any exception.

@dbulahov
test for wrong table
62d46a8 
Signed-off-by: dbulahov <[email protected]>
@dbulahov
test for wrong table
d9f8407 
Signed-off-by: dbulahov <[email protected]>
@dbulahov
test for wrong table
f8471d0 
Signed-off-by: dbulahov <[email protected]>
@dbulahov
test for wrong column
60bad26 
Signed-off-by: dbulahov <[email protected]>
manticore-projects
manticore-projects requested changes Nov 22, 2024
View reviewed changes
src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java
}

@Test
void testWithWrongColumn() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not throw any exception because foo.name1 is not related to any SelectItem and not part of the lineage.

Copy link
Author

Choose a reason for hiding this comment

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

we throw exception if we use "wrong" name of column. But why "wrong" table name should have other behavior

src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java

//should be exception because foo1 table is not exist
org.junit.jupiter.api.Assertions.assertThrows(JSQLParserException.class, () -> JSQLColumResolver.getResultSetMetaData("select sum(foo1.id) from foo group by foo.name", metaData));
org.junit.jupiter.api.Assertions.assertThrows(JSQLParserException.class, () -> JSQLColumResolver.getResultSetMetaData("select avg(foo1.id) from foo group by foo.name", metaData));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Literally the same tests.

src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java
.addTable("foo", new JdbcColumn("id"), new JdbcColumn("name"));

//should be exception because foo1 table is not exist
org.junit.jupiter.api.Assertions.assertThrows(JSQLParserException.class, () -> JSQLColumResolver.getResultSetMetaData("select sum(foo1.id) from foo group by foo.name", metaData));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please:

import org.junit.jupiter.api.Assertions
and then call Assertions.assertThrows(...)

src/test/java/ai/starlake/transpiler/JSQLColumnResolverTest.java
.addTable("b", new JdbcColumn("col1"), new JdbcColumn("col2"), new JdbcColumn("col3"));

//should be exception because bb table is not exist
org.junit.jupiter.api.Assertions.assertThrows(JSQLParserException.class, () -> JSQLColumResolver.getResultSetMetaData("select * from a where a.col1 in (select bb.col1 from bb)", metaData));
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSQLParserException is very generic and can mean anything.
If you want to make this test meaniningful then please check if Table bb is pointed out and blamed correctly.

@stbischof
Copy link
Contributor

Pritty relevant that we can resolve any column in the Statement or an inner statement in from section or functions or elsewhere

@stbischof
Copy link
Contributor

stbischof commented Nov 27, 2024 •
edited
Loading

Should we have a meeting on that?

I teally would like to have this part in. And if we need a common Unserstanding of what we expect , a discussion may help.

@manticore-projects
Copy link
Collaborator

Should we have a meeting on that?

I teally would like to have this part in. And if we need a common Unserstanding of what we expect , a discussion may help.

That's a good idea. I can offer a time slot for Friday or Saturday between 7 AM and 3 PM GMT. Please do let me know if this suits you.

@stbischof
Copy link
Contributor

stbischof commented Nov 27, 2024 •
edited
Loading

Friday 7.00 GMT
@dbulahov

Also okay for you?

@manticore-projects
Copy link
Collaborator

Friday 7.00 GMT @dbulahov

Also okay for you?

Join Zoom Meeting
https://zoom.us/j/94360906149?pwd=THVV4ug1Juvw3dakwx2MG63VIHQjsS.1

Meeting ID: 943 6090 6149
Passcode: 194186

@stbischof
Copy link
Contributor

some usecases

we have database with 
 tables:
    t1exist (int t1c1exist, varchar(50) t1c2exist)
      1, test
      2, test
      3, test1		    	
	
    fooFact (int t2c1exist, varchar(50) t21c2exist, double t2c3exist)
      1, test, 10
      2, test, 20
      3, test1, 40

"select * from foo where t1exist.id in (select t2miss.t2c1exist from t2miss)"  we are expecting exception if t2miss absent in describing

"select sum(t1miss.t1c1exist) from t1exist group by t1exist.t1c2exist"  we are expecting exception if t1miss table absent in describing. it relates any aggregation functions (max, min, avg, count and e.c.t.)

"select trim(t1miss.t1c2exist)  from t1exist" we are expecting exception if t1miss table absent in describing.  but "select trim(t1exist.t1c2miss)  from t1exist" is working. we have exception when we use wrong column name.   

"select sum(t1exist.t1c1exist) FROM t1exist GROUP BY t1exist.t1c2exist HAVING sum(t1miss.t1c1exist) > 5" we are expecting exception if t1miss table absent in describing.

"select sum(t1exist.t1c1exist) from foo group by t1exist.t1c2exist having t1miss.t1c2exist = 'test'" we are expecting exception if t1miss table absent in describing.

"select sum(t1exist.t1c1exist) from foo group by t1exist.t1c2exist having t1exist.t1c2miss = 'test'"  we are expecting exception if column with t1c2miss absent in describing. 

"select max(select t2c1exist from t2miss), t1exist.t1c2exist. from t1exist" we are expecting exception if table t2miss absent in describing.

"select max(select t2c1exist from t2exist), t1miss.t1c2exist from t1exist" we are expecting exception if table t1miss absent in describing.

@manticore-projects
Copy link
Collaborator

Greetings @stbischof and thank you for the examples.

All of those can be covered (only) when we extended the scope of lineage/resolution to the full AST.

At the moment, by design, only SelecrItems are resolved.

@stbischof
Copy link
Contributor

Will contact you in next days.

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

@manticore-projects manticore-projects manticore-projects requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbulahov @stbischof @manticore-projects