[CALCITE-2491] Refactor NameSet, NameMap, and NameMultimap - #815
Closed
vlsi wants to merge 1 commit into
Closed
Conversation
eolivelli
suggested changes
Aug 31, 2018
eolivelli
left a comment
Contributor
There was a problem hiding this comment.
Awesome work.
What about adding unit tests to cover this new utilities?
Contributor
Author
|
@eolivelli , It adds no features, it removes no features, it fixes no bugs, that is why I assume existing tests (e.g. Do you think tests are required? |
Contributor
Author
|
Well, it does fix a concurrency bug in NameHelper (by removing the class :) ), and the bug is easily reproducible in CI: https://builds.apache.org/job/Calcite-Master/807/jdk=JDK%2010%20(latest),label_exp=ubuntu&&!cloud-slave/console Hopefully after this PR gets merged the bug will go away. |
eolivelli
approved these changes
Aug 31, 2018
eolivelli
left a comment
Contributor
There was a problem hiding this comment.
Okay.
I did not notice Julian's test suite.
Your implementation is cleaner and it looks good to me
+1
Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :)
pengzhiwei2018
pushed a commit
to pengzhiwei2018/calcite
that referenced
this pull request
Sep 11, 2018
[CALCITE-2462] RexProgramTest: replace nullLiteral->nullInt, unknownLiteral->nullBool for brevity Update Michael Mior's affiliation Site: Update Julian Hyde's affiliation [CALCITE-2480] NameSet.contains wrongly returns false when element in set is upper-case and seek is lower-case Implement toString, hashCode and equals for NameSet, NameMap and NameMultimap; and add tests. Fix some cosmetic issues in other code, such as missing newline at end of file. [CALCITE-2467] Upgrade owasp-dependency-check maven plugin to 3.3.1 [CALCITE-2481] NameSet assumes lower-case characters have greater codes, which does not hold for certain characters Includes EquivalenceSet, a nice general-purpose class to compute reflexive, symmetric, transitive closure. [CALCITE-2271] Join of two views with window aggregates produces incorrect results or NPE Avoid NPE in BlockBuilder.append when empty variable initializer is used closes apache#673 [CALCITE-2327] Avoid simplification of x AND NOT(x) to false for nullable x x AND NOT(x) ==> FALSE AND x IS NULL closes apache#707 [CALCITE-311] Added a test-case for filter after window aggregate Filters should not be pushed through window aggregate since it might impact results. [CALCITE-2494] RexFieldAccess should implement equals/hashCode [CALCITE-2327] RexSimplify: AND(x, y, NOT(y)) ==> AND(x, null, IS NULL(y)) [CALCITE-2497] Update Janino version to 3.0.9 [CALCITE-2495] Support encoded URLs in calcite.util.Source, and use it for URL->File conversion in tests For the record: 1) URL.getPath() produces %20, so it is added to forbidden signatures 2) Paths.get(url.toURI()).toFile() almost works, however it fails with URL is not hierarchical for new URL("file:test.java") 3) new File(URL.toURI() is worse than apache#2 4) URLDecoder must not be used to decode %20, since it will convert + to spaces as well, thus it will corrupt test.c++ filenames 5) It looks like url.toURI().getSchemeSpecificPart()) properly handles "opaque" URIs (which are relative file:test.java kind of URLs) 6) file:/test is relative for Windows, and absolute would be like file:c:/test MockCatalogReader is used in testing, so cache should be disabled there to avoid thread conflicts and/or stale results Support AND, OR, COALESCE, IS...DISTINCT in RexUtil#op [CALCITE-2505] Add ignored test for fail in RexSimplify with IS DISTINCT FROM Simplification of isFalse(isNotDistinctFrom(vBool(0), vBool(1))) causes AssertionError: wrong operand count 1 for IS DISTINCT FROM [CALCITE-2506] Add ignored test for fail in RexSimplify with coalesce coalesce(unaryPlus(nullInt), unaryPlus(vInt())) fails with result mismatch: when applied to {?0.int0=-1}, COALESCE(+(null), +(?0.int0)) yielded -1, and +(null) yielded NULL [CALCITE-2483] Druid adapter, when querying Druid segment metadata, throws when row number is larger than Integer.MAX_VALUE (Hongze Zhang) Close apache#799 [CALCITE-2469] RexSimplify should optimize '(NOT x) IS NULL' to 'x IS NULL' (pengzhiwei) Previously it optimized '(NOT x) IS NULL' to 'x IS NOT NULL', which is wrong. Generalize the above, to simplify 'f(x) IS NULL' to 'x IS NULL' for any operator 'f' that is known to be strong. (Julian Hyde) Close apache#796 [CALCITE-1026] Allow models in YAML format Make JSON/YAML mappers static final. Add tests to check model inline: json and yaml and to check model uri to hson/yaml files. Close apache#791 [CALCITE-2486] Upgrade Apache parent POM to version 21 Also upgrade commons-dbcp2 to 2.5.0, commons-lang3 to 3.8, commons-pool to 2.6.0, esri-geometry-api to 2.2.0, h2 to 1.4.197, hsqldb to 2.4.1, httpclient to 2.5.6, httpcore to 4.4.10, java-diff to 1.1.2, jsoup to 1.11.3, maven-dependency-plugin to 3.1.1, maven-enforcer-plugin to 3.0.0-M2, mockito to 2.21.0. [CALCITE-2470] In RelBuilder, project method should combine expressions if the underlying node is a Project Add RelBuilder.shouldMergeProject() to allow sub-classes to disable merging. Improve the message given by CompositeMatcher when match fails. When RelStructuredTypeFlattener rewrites a RexInputRef be sure to use the field's new type. (It might have strengthened from say INTEGER to INTEGER NOT NULL.) Add a test case inspired by Drill (it passes in Calcite, but I gather it fails in Drill). [CALCITE-1026] Fix ModelTest#testYamlFileDetection when source folder has spaces [CALCITE-1026] Remove unused import [CALCITE-2520] Make SparkHandlerImpl#compile silent by default, print code in calcite.debug=true mode only [CALCITE-2519] Silence ERROR logs from CalciteException, SqlValidatorException during tests Use threadCount=1/perCoreThreadCount=false to disable parallel test execution at Travis Previous option -Dsurefire.parallel= made JUnit to log just CalciteSuite details, thus the test looked like a stuck one fixes apache#816 Skip second Checkstyle execution during Travis CI build Checkstyle is validated during mvn install part, so there's no need to repeat that [CALCITE-2491] Refactor NameSet, NameMap, and NameMultimap Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815 [CALCITE-2512] Move StreamTest#ROW_GENERATOR to Table.scan().iterator to make it not shared between threads (Sergey Nuyanzin) fixes apache#813 [CALCITE-2498] fix bug when geode adapter quotes booleans as strings (Andrei Sereda) GeodeFilter was incorrectly quoting boolean literals as SQL strings ('true' instead of true) fixes apache#809 [CALCITE-2522] Remove e.printStackTrace() from CalciteAssert#returns [CALCITE-2514] Add SqlIdentifier conversion to ITEM operator for dynamic tables in ExtendedExpander (Arina Ielchiieva) Close apache#814 Following [CALCITE-2469] simplify "f(x, y) IS NULL" to "x IS NULL OR y IS NULL" if "f" is strong Previously we did not use OR, which was wrong. [CALCITE-2418] Remove matchRecognize field of SqlSelect [CALCITE-2433] SqlAdvisor: support configurable quoting characters [CALCITE-2434] SqlAdvisor: support hints for nested tables/schemas [CALCITE-2473] SqlAdvisor: support -- comments [CALCITE-2474] SqlAdvisor: avoid NPE in lookupFromHints where FROM is empty [CALCITE-2475] SqlAdvisor: support MINUS [CALCITE-2476] SqlAdvisor: produce hints when sub-query with * is present in query simplifySql produced 0 AS "*", and it caused the SQL to fail validation. The fix is to disable transformation of expressions to "0 as ...", so it keeps select * as is. [CALCITE-2477] SqlAdvisor: scalar sub-query support [CALCITE-2478] SqlAdvisor: purge from_clause when _suggest_ token is located in one of the from sub-queries [CALCITE-2479] SqlAdvisor: automatically quote identifiers that look like SQL keywords [CALCITE-2484] Move dynamic tests to a separate class like SqlValidatorDynamicTest, and avoid reuse of MockCatalogReaderDynamic [CALCITE-2484] Add SqlValidatorDynamicTest to CalciteSuite [CALCITE-2521] Guard RelMetadataTest#testMetadataHandlerCacheLimit with CalciteAssert.ENABLE_SLOW The test does not reproduce the original bug, and it is very slow to execute. [CALCITE-2523] Guard PartiallyOrderedSetTest#testPosetBitsLarge with CalciteAssert.ENABLE_SLOW Reduce HepPlannerTest#testRuleApplyCount complexity [CALCITE-2543] Upgrade SQLLine to 1.5.0 [CALCITE-2112] Add Maven wrapper for Calcite (Ratandeep S. Ratti) Update doc, remove maven jar; add jar to .gitignore (Julian Hyde) Use TLS 1.2 on windows (Sergey Nuyanzin) [CALCITE-2412] Add Windows CI via AppVeyor (Sergey Nuyanzin) fixes apache#757
pengzhiwei2018
pushed a commit
to pengzhiwei2018/calcite
that referenced
this pull request
Sep 17, 2018
[CALCITE-2462] RexProgramTest: replace nullLiteral->nullInt, unknownLiteral->nullBool for brevity Update Michael Mior's affiliation Site: Update Julian Hyde's affiliation [CALCITE-2480] NameSet.contains wrongly returns false when element in set is upper-case and seek is lower-case Implement toString, hashCode and equals for NameSet, NameMap and NameMultimap; and add tests. Fix some cosmetic issues in other code, such as missing newline at end of file. [CALCITE-2467] Upgrade owasp-dependency-check maven plugin to 3.3.1 [CALCITE-2481] NameSet assumes lower-case characters have greater codes, which does not hold for certain characters Includes EquivalenceSet, a nice general-purpose class to compute reflexive, symmetric, transitive closure. [CALCITE-2271] Join of two views with window aggregates produces incorrect results or NPE Avoid NPE in BlockBuilder.append when empty variable initializer is used closes apache#673 [CALCITE-2327] Avoid simplification of x AND NOT(x) to false for nullable x x AND NOT(x) ==> FALSE AND x IS NULL closes apache#707 [CALCITE-311] Added a test-case for filter after window aggregate Filters should not be pushed through window aggregate since it might impact results. [CALCITE-2494] RexFieldAccess should implement equals/hashCode [CALCITE-2327] RexSimplify: AND(x, y, NOT(y)) ==> AND(x, null, IS NULL(y)) [CALCITE-2497] Update Janino version to 3.0.9 [CALCITE-2495] Support encoded URLs in calcite.util.Source, and use it for URL->File conversion in tests For the record: 1) URL.getPath() produces %20, so it is added to forbidden signatures 2) Paths.get(url.toURI()).toFile() almost works, however it fails with URL is not hierarchical for new URL("file:test.java") 3) new File(URL.toURI() is worse than apache#2 4) URLDecoder must not be used to decode %20, since it will convert + to spaces as well, thus it will corrupt test.c++ filenames 5) It looks like url.toURI().getSchemeSpecificPart()) properly handles "opaque" URIs (which are relative file:test.java kind of URLs) 6) file:/test is relative for Windows, and absolute would be like file:c:/test MockCatalogReader is used in testing, so cache should be disabled there to avoid thread conflicts and/or stale results Support AND, OR, COALESCE, IS...DISTINCT in RexUtil#op [CALCITE-2505] Add ignored test for fail in RexSimplify with IS DISTINCT FROM Simplification of isFalse(isNotDistinctFrom(vBool(0), vBool(1))) causes AssertionError: wrong operand count 1 for IS DISTINCT FROM [CALCITE-2506] Add ignored test for fail in RexSimplify with coalesce coalesce(unaryPlus(nullInt), unaryPlus(vInt())) fails with result mismatch: when applied to {?0.int0=-1}, COALESCE(+(null), +(?0.int0)) yielded -1, and +(null) yielded NULL [CALCITE-2483] Druid adapter, when querying Druid segment metadata, throws when row number is larger than Integer.MAX_VALUE (Hongze Zhang) Close apache#799 [CALCITE-2469] RexSimplify should optimize '(NOT x) IS NULL' to 'x IS NULL' (pengzhiwei) Previously it optimized '(NOT x) IS NULL' to 'x IS NOT NULL', which is wrong. Generalize the above, to simplify 'f(x) IS NULL' to 'x IS NULL' for any operator 'f' that is known to be strong. (Julian Hyde) Close apache#796 [CALCITE-1026] Allow models in YAML format Make JSON/YAML mappers static final. Add tests to check model inline: json and yaml and to check model uri to hson/yaml files. Close apache#791 [CALCITE-2486] Upgrade Apache parent POM to version 21 Also upgrade commons-dbcp2 to 2.5.0, commons-lang3 to 3.8, commons-pool to 2.6.0, esri-geometry-api to 2.2.0, h2 to 1.4.197, hsqldb to 2.4.1, httpclient to 2.5.6, httpcore to 4.4.10, java-diff to 1.1.2, jsoup to 1.11.3, maven-dependency-plugin to 3.1.1, maven-enforcer-plugin to 3.0.0-M2, mockito to 2.21.0. [CALCITE-2470] In RelBuilder, project method should combine expressions if the underlying node is a Project Add RelBuilder.shouldMergeProject() to allow sub-classes to disable merging. Improve the message given by CompositeMatcher when match fails. When RelStructuredTypeFlattener rewrites a RexInputRef be sure to use the field's new type. (It might have strengthened from say INTEGER to INTEGER NOT NULL.) Add a test case inspired by Drill (it passes in Calcite, but I gather it fails in Drill). [CALCITE-1026] Fix ModelTest#testYamlFileDetection when source folder has spaces [CALCITE-1026] Remove unused import [CALCITE-2520] Make SparkHandlerImpl#compile silent by default, print code in calcite.debug=true mode only [CALCITE-2519] Silence ERROR logs from CalciteException, SqlValidatorException during tests Use threadCount=1/perCoreThreadCount=false to disable parallel test execution at Travis Previous option -Dsurefire.parallel= made JUnit to log just CalciteSuite details, thus the test looked like a stuck one fixes apache#816 Skip second Checkstyle execution during Travis CI build Checkstyle is validated during mvn install part, so there's no need to repeat that [CALCITE-2491] Refactor NameSet, NameMap, and NameMultimap Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815 [CALCITE-2512] Move StreamTest#ROW_GENERATOR to Table.scan().iterator to make it not shared between threads (Sergey Nuyanzin) fixes apache#813 [CALCITE-2498] fix bug when geode adapter quotes booleans as strings (Andrei Sereda) GeodeFilter was incorrectly quoting boolean literals as SQL strings ('true' instead of true) fixes apache#809 [CALCITE-2522] Remove e.printStackTrace() from CalciteAssert#returns [CALCITE-2514] Add SqlIdentifier conversion to ITEM operator for dynamic tables in ExtendedExpander (Arina Ielchiieva) Close apache#814 Following [CALCITE-2469] simplify "f(x, y) IS NULL" to "x IS NULL OR y IS NULL" if "f" is strong Previously we did not use OR, which was wrong. [CALCITE-2418] Remove matchRecognize field of SqlSelect [CALCITE-2433] SqlAdvisor: support configurable quoting characters [CALCITE-2434] SqlAdvisor: support hints for nested tables/schemas [CALCITE-2473] SqlAdvisor: support -- comments [CALCITE-2474] SqlAdvisor: avoid NPE in lookupFromHints where FROM is empty [CALCITE-2475] SqlAdvisor: support MINUS [CALCITE-2476] SqlAdvisor: produce hints when sub-query with * is present in query simplifySql produced 0 AS "*", and it caused the SQL to fail validation. The fix is to disable transformation of expressions to "0 as ...", so it keeps select * as is. [CALCITE-2477] SqlAdvisor: scalar sub-query support [CALCITE-2478] SqlAdvisor: purge from_clause when _suggest_ token is located in one of the from sub-queries [CALCITE-2479] SqlAdvisor: automatically quote identifiers that look like SQL keywords [CALCITE-2484] Move dynamic tests to a separate class like SqlValidatorDynamicTest, and avoid reuse of MockCatalogReaderDynamic [CALCITE-2484] Add SqlValidatorDynamicTest to CalciteSuite [CALCITE-2521] Guard RelMetadataTest#testMetadataHandlerCacheLimit with CalciteAssert.ENABLE_SLOW The test does not reproduce the original bug, and it is very slow to execute. [CALCITE-2523] Guard PartiallyOrderedSetTest#testPosetBitsLarge with CalciteAssert.ENABLE_SLOW Reduce HepPlannerTest#testRuleApplyCount complexity [CALCITE-2543] Upgrade SQLLine to 1.5.0 [CALCITE-2112] Add Maven wrapper for Calcite (Ratandeep S. Ratti) Update doc, remove maven jar; add jar to .gitignore (Julian Hyde) Use TLS 1.2 on windows (Sergey Nuyanzin) [CALCITE-2412] Add Windows CI via AppVeyor (Sergey Nuyanzin) fixes apache#757 [CALCITE-2505] Fix assertion error when simplifying is [not] distinct expressions (Haisheng Yuan) Sample expression that caused failure: isFalse(isNotDistinctFrom(vBool(0), vBool(1))) closes apache#828 Increase timeout for Cassandra daemon startup for CassandraAdapterTest Remove redundant 'new' expression in constant array creation add simplifyStrong for generic simplification for operand with null add comment for policy might be null make policy() return AS_IS by default remove unused null test simplify duplication code remove unused empty line fix duplicate call of simplify operand add comment for simplifyStrong
F21
pushed a commit
to F21/calcite
that referenced
this pull request
Jan 3, 2019
Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815
wangxlong
pushed a commit
to wangxlong/calcite
that referenced
this pull request
Feb 13, 2020
Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815
jamesstarr
pushed a commit
to jamesstarr/calcite
that referenced
this pull request
Aug 28, 2025
Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815 Change-Id: I8297845fcee6bcca8ea01d130e55bef047541263
jamesstarr
pushed a commit
to jamesstarr/calcite
that referenced
this pull request
Mar 16, 2026
Collections are thread-safe when used in read-only scenarios. Name*.immutableCopyOf are always thread-safe as writes are prohibited :) fixes apache#815 Change-Id: I8297845fcee6bcca8ea01d130e55bef047541263
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
https://issues.apache.org/jira/browse/CALCITE-2491
Collections are thread-safe when used in read-only scenarios.
Name*.immutableCopyOf are always thread-safe as writes are prohibited :)