Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1667

Forbid calls to JDK APIs that use the default locale, time zone or character set

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: avatica-1.10.0, 1.12.0
    • Component/s: avatica, core
    • Labels:
      None

      Description

      Calcite and Avatica currently have multiple failures when run using the Turkish locale.

      This was found with SOLR-8593. My steps to try to reproduce are posted in a comment below.

      Here is the Solr log:

      Policeman Jenkins found a reproducing seed for a TestSQLHandler failure https://jenkins.thetaphi.de/job/Lucene-Solr-master-Linux/19059/:
        [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestSQLHandler -Dtests.method=doTest -Dtests.seed=F875A6E80D435C44 -Dtests.multiplier=3 -Dtests.slow=true -Dtests.locale=tr -Dtests.timezone=Africa/Lagos -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
        [junit4] ERROR   26.3s J0 | TestSQLHandler.doTest <<<
        [junit4]    > Throwable #1: java.io.IOException: --> https://127.0.0.1:37214/collection1:Failed to execute sqlQuery 'select str_s, count(*), sum(field_i), min(field_i), max(field_i), cast(avg(1.0 * field_i) as float) from collection1 where text='XXXX' group by str_s order by sum(field_i) asc limit 2' against JDBC connection 'jdbc:calcitesolr:'.
        [junit4]    > Error while executing SQL "select str_s, count(*), sum(field_i), min(field_i), max(field_i), cast(avg(1.0 * field_i) as float) from collection1 where text='XXXX' group by str_s order by sum(field_i) asc limit 2": From line 1, column 39 to line 1, column 50: No match found for function signature min(<NUMERIC>)
        [junit4]    > 	at __randomizedtesting.SeedInfo.seed([F875A6E80D435C44:5F311E4C60F84FFD]:0)
        [junit4]    > 	at org.apache.solr.client.solrj.io.stream.SolrStream.read(SolrStream.java:235)
        [junit4]    > 	at org.apache.solr.handler.TestSQLHandler.getTuples(TestSQLHandler.java:2325)
        [junit4]    > 	at org.apache.solr.handler.TestSQLHandler.testBasicGrouping(TestSQLHandler.java:651)
        [junit4]    > 	at org.apache.solr.handler.TestSQLHandler.doTest(TestSQLHandler.java:77)
        [junit4]    > 	at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsFixedStatement.callStatement(BaseDistributedSearchTestCase.java:985)
        [junit4]    > 	at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsStatement.evaluate(BaseDistributedSearchTestCase.java:960)
      [...]
        [junit4]   2> NOTE: test params are: codec=Asserting(Lucene70): {n_l1=PostingsFormat(name=LuceneFixedGap), multiDefault=BlockTreeOrds(blocksize=128), Field_i=BlockTreeOrds(blocksize=128), intDefault=PostingsFormat(name=LuceneFixedGap), n_dt1=BlockTreeOrds(blocksize=128), n_td1=BlockTreeOrds(blocksize=128), Str_s=Lucene50(blocksize=128), n_d1=PostingsFormat(name=LuceneFixedGap), field_i=BlockTreeOrds(blocksize=128), str_s=Lucene50(blocksize=128), n_f1=BlockTreeOrds(blocksize=128), n_ti1=FST50, rnd_b=FST50, n_tl1=BlockTreeOrds(blocksize=128), _version_=PostingsFormat(name=LuceneFixedGap), n_tf1=PostingsFormat(name=LuceneFixedGap), n_tdt1=PostingsFormat(name=LuceneFixedGap), id=FST50, text=Lucene50(blocksize=128), Text_t=PostingsFormat(name=LuceneFixedGap), timestamp=PostingsFormat(name=LuceneFixedGap)}, docValues:{_version_=DocValuesFormat(name=Lucene70), multiDefault=DocValuesFormat(name=Asserting), Field_i=DocValuesFormat(name=Asserting), intDefault=DocValuesFormat(name=Lucene70), id=DocValuesFormat(name=Memory), Str_s=DocValuesFormat(name=Direct), field_i=DocValuesFormat(name=Asserting), str_s=DocValuesFormat(name=Direct), n_f1=DocValuesFormat(name=Asserting)}, maxPointsInLeafNode=197, maxMBSortInHeap=6.686785692870154, sim=RandomSimilarity(queryNorm=false): {}, locale=tr, timezone=Africa/Lagos
        [junit4]   2> NOTE: Linux 4.4.0-53-generic i386/Oracle Corporation 1.8.0_121 (32-bit)/cpus=12,threads=1,free=50683592,total=254599168
      

      and it contains the Calcite message No match found for function signature min(<NUMERIC>).

        Issue Links

          Activity

          Hide
          risdenk Kevin Risden added a comment -

          From calcite repo directory:

          MAVEN_OPTS="$MAVEN_OPTS -Duser.locale=tr" mvn clean test
          

          Also tried editing the pom.xml in the calcite directory for Surefire and added "-Duser.locale=tr" to the surefire plugin argLine.

          I did the same steps from the avatica subdirectory.

          I ran "mvn clean test" with each of my changes.

          Show
          risdenk Kevin Risden added a comment - From calcite repo directory: MAVEN_OPTS= "$MAVEN_OPTS -Duser.locale=tr" mvn clean test Also tried editing the pom.xml in the calcite directory for Surefire and added "-Duser.locale=tr" to the surefire plugin argLine. I did the same steps from the avatica subdirectory. I ran "mvn clean test" with each of my changes.
          Hide
          julianhyde Julian Hyde added a comment -

          I can reproduce it. Running with the command

          MAVEN_OPTS="$MAVEN_OPTS -Duser.language=tr -Duser.country=TR" mvn clean test
          

          you get 106 failures and 1310 errors. The solution is to use String.toLowerCase(Locale) and String.toUpperCase(Locale) rather than String.toLowerCase() and String.toUpperCase().

          I suggest we go further, and use the forbidden-apis maven plugin. It will identify all places where we call JDK methods that use the JVM's locale, time zone or character set. This is basically what you want if you are writing a server. Apache Lucene already uses it; see LUCENE-4753.

          We will need to fix Avatica also; Avatica's ConnectionConfigImpl.parse calls String.toUpperCase, and this may prevent connect-string properties from being recognized in both Calcite and Avatica.

          Show
          julianhyde Julian Hyde added a comment - I can reproduce it. Running with the command MAVEN_OPTS= "$MAVEN_OPTS -Duser.language=tr -Duser.country=TR" mvn clean test you get 106 failures and 1310 errors. The solution is to use String.toLowerCase(Locale) and String.toUpperCase(Locale) rather than String.toLowerCase() and String.toUpperCase() . I suggest we go further, and use the forbidden-apis maven plugin. It will identify all places where we call JDK methods that use the JVM's locale, time zone or character set. This is basically what you want if you are writing a server. Apache Lucene already uses it; see LUCENE-4753 . We will need to fix Avatica also; Avatica's ConnectionConfigImpl.parse calls String.toUpperCase , and this may prevent connect-string properties from being recognized in both Calcite and Avatica.
          Hide
          risdenk Kevin Risden added a comment - - edited

          I have a start on trying to add forbidden apis if it would help. I started down that path of forbidden apis thinking it would at least be a start.

          My work is pushed here so far:

          https://github.com/risdenk/calcite/tree/forbiddenapis

          I don't know if its necessarily correct, but just started down the path of trying to fix many of the forbiddenapi errors.

          Show
          risdenk Kevin Risden added a comment - - edited I have a start on trying to add forbidden apis if it would help. I started down that path of forbidden apis thinking it would at least be a start. My work is pushed here so far: https://github.com/risdenk/calcite/tree/forbiddenapis I don't know if its necessarily correct, but just started down the path of trying to fix many of the forbiddenapi errors.
          Hide
          julianhyde Julian Hyde added a comment -

          I've been working on this also. My work is in https://github.com/julianhyde/calcite/tree/1667-forbidden-apis. I think it's complete (but I need to run the test suite). Sorry we seem to have overlapped work; I only just noticed your comment.

          Show
          julianhyde Julian Hyde added a comment - I've been working on this also. My work is in https://github.com/julianhyde/calcite/tree/1667-forbidden-apis . I think it's complete (but I need to run the test suite). Sorry we seem to have overlapped work; I only just noticed your comment.
          Hide
          risdenk Kevin Risden added a comment -

          No worries. I took a look over what you had and it looks like the same path I was going down. The only question I had was if the signature files are necessary. The bundled signatures should be the right thing without needing to specify the additional extra signatures.

          Show
          risdenk Kevin Risden added a comment - No worries. I took a look over what you had and it looks like the same path I was going down. The only question I had was if the signature files are necessary. The bundled signatures should be the right thing without needing to specify the additional extra signatures.
          Hide
          julianhyde Julian Hyde added a comment -

          Agreed, the bundled signatures take care of String.toLowerCase and .toUpperCase, which was the original goal. I had those in the custom signatures.txt but I've taken them out. But it's useful to have a custom file: I have used it to control how people call System.exit, StringBuffer and some other things. I'll commit shortly, when the tests have run through.

          Show
          julianhyde Julian Hyde added a comment - Agreed, the bundled signatures take care of String.toLowerCase and .toUpperCase, which was the original goal. I had those in the custom signatures.txt but I've taken them out. But it's useful to have a custom file: I have used it to control how people call System.exit, StringBuffer and some other things. I'll commit shortly, when the tests have run through.
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/8df27ce9 (avatica), http://git-wip-us.apache.org/repos/asf/calcite/commit/75152c5b (calcite).

          Some of the issues in Avatica appear in Calcite, so Calcite will only be fully fixed when it upgrades to avatica-1.10 (see CALCITE-1612), which will probably happen in the calcite-1.13 release. CalciteConnectionProperty.parse2 is a workaround that can be removed after that upgrade.

          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/8df27ce9 (avatica), http://git-wip-us.apache.org/repos/asf/calcite/commit/75152c5b (calcite). Some of the issues in Avatica appear in Calcite, so Calcite will only be fully fixed when it upgrades to avatica-1.10 (see CALCITE-1612 ), which will probably happen in the calcite-1.13 release. CalciteConnectionProperty.parse2 is a workaround that can be removed after that upgrade.
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              risdenk Kevin Risden
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development