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.

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

Remove upper case transformations from GA #28

Merged
merged 7 commits into from
Apr 9, 2018
Merged

Conversation

danielvangelder
Copy link
Contributor

@danielvangelder danielvangelder commented Apr 3, 2018

GA transformed table and column names to upper case names which would cause trouble in unit test generation. Removing this feature does not seem to cause any trouble for Sqlfpc or jSQLparser (for which it was intended). See #29 for details.

pvdstel
pvdstel previously approved these changes Apr 3, 2018
@codecov
Copy link

codecov bot commented Apr 3, 2018

Codecov Report

Merging #28 into master will increase coverage by 0.53%.
The diff coverage is 96.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #28      +/-   ##
============================================
+ Coverage     59.29%   59.82%   +0.53%     
- Complexity      669      675       +6     
============================================
  Files            63       63              
  Lines          2823     2823              
  Branches        394      394              
============================================
+ Hits           1674     1689      +15     
+ Misses         1050     1040      -10     
+ Partials         99       94       -5
Impacted Files Coverage Δ Complexity Δ
...tudelft/serg/evosql/metaheuristics/GAApproach.java 73.33% <0%> (ø) 21 <0> (ø) ⬇️
...ft/serg/evosql/sql/parser/UsedColumnExtractor.java 88.88% <100%> (ø) 3 <1> (ø) ⬇️
...n/java/nl/tudelft/serg/evosql/sql/TableSchema.java 46.42% <100%> (ø) 14 <6> (ø) ⬇️
...elft/serg/evosql/sql/parser/SqlSecurerVisitor.java 51.48% <100%> (ø) 45 <3> (ø) ⬇️
.../evosql/sql/parser/UsedColumnExtractorVisitor.java 72.25% <100%> (+3.25%) 99 <2> (+5) ⬆️
...elft/serg/evosql/sql/parser/TablesNamesFinder.java 34.72% <100%> (ø) 42 <2> (ø) ⬇️
...ava/nl/tudelft/serg/evosql/fixture/FixtureRow.java 71.73% <100%> (ø) 14 <2> (ø) ⬇️
.../nl/tudelft/serg/evosql/fixture/type/DBDouble.java 84.9% <0%> (+3.77%) 17% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51aaf2d...7adffb5. Read the comment docs.

@pvdstel pvdstel dismissed their stale review April 3, 2018 11:26

My own changes should also be reviewed.

@pvdstel
Copy link
Contributor

pvdstel commented Apr 3, 2018

Currently, the following three tests take way longer to execute than they used to:

  • realQueryBasedOnErpnextQ1016P19
  • testStringFunctionsReverse
  • refrigeratorReverse

I'm not sure what causes it, but we should look into this.

@pvdstel
Copy link
Contributor

pvdstel commented Apr 6, 2018

An update to my own concerns about the test speed regression: it appears that the tests run as fast as ever when using CLI tools such as Gradle to run the test. It mostly seems that the new IntelliJ 2018.1 release is slowing things down.

While checking this out, Gradle generated 321735 log lines in a timespan of 86.063 seconds when running unit tests in StringsTest.java. Some tests in IDEA did not even finish, and the whole testing process seemed to take longer for the tests that did finish. Perhaps some new instrumentation was added to the new release that makes the test performance drop a bit. I don't think this is a blocking issue however.

Edit: it's not the upgrade from IntelliJ 2017.3 to 2018.1 since it's slow in there as well.

@mauricioaniche
Copy link
Contributor

Wow, @danielvangelder, this is a lot of work. Thanks for this PR!

Just for me to understand the motives: why the upper case would cause problems to the test generation?

@pvdstel
Copy link
Contributor

pvdstel commented Apr 6, 2018

We started testing using a simple PostgreSQL server, since that's a nice free SQL DBMS that supports escaping via quotes ("). This was necessary since the SqlSecurer class escapes all identifiers (table and column names) with quotes. This is probably necessary since SQLFpc doesn't like MySQL syntax with the backticks (`). Also, we both think it's good to escape identifiers but that's beside the point.

Now, what happened is that we created a schema in Postgres that was using camelCase for identifiers. SQL is usually case-insensitive, but once you start escaping identifiers, the DBMS suddenly decides that it should now be case-sensitive, and then this becomes an issue.

To illustrate; we have a table named demoTest. Whether you use DEMOTEST or demotest does not matter. However, EvoSQL currently transforms these identifiers into "DEMOTEST" which does not exist in a case-sensitive system. This made Postgres spit out all kinds of nasty errors. We figured not making identifiers upper case would be preferable to having the user update their database schema to uppercase notation 😉

@mauricioaniche
Copy link
Contributor

Makes totally sense to me!

Copy link
Contributor Author

@danielvangelder danielvangelder left a comment

Choose a reason for hiding this comment

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

Good work Paul on fixing the test cases relying on case sensitivity!

Copy link
Contributor

@pvdstel pvdstel left a comment

Choose a reason for hiding this comment

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

Good job on your part as well!

@pvdstel pvdstel merged commit 5875142 into master Apr 9, 2018
@pvdstel pvdstel deleted the case-sensitivity branch April 9, 2018 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants