Skip to content

fix bug when rewriting sql virtual column registry#12718

Merged
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:fix-virtual-column-registry-concurrent-modification
Jul 1, 2022
Merged

fix bug when rewriting sql virtual column registry#12718
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:fix-virtual-column-registry-concurrent-modification

Conversation

@clintropolis
Copy link
Member

Description

Fixes a bug in #12241 from a simple mistake I made that results in modifying the Map that is being iterated over ending up in a ConcurrentModificationException. The added test fails to plan due to this error prior to the changes in this PR, with one of these in the stack trace:

Caused by: java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextNode(HashMap.java:1469)
	at java.util.HashMap$EntryIterator.next(HashMap.java:1503)
	at java.util.HashMap$EntryIterator.next(HashMap.java:1501)
	at org.apache.druid.sql.calcite.rel.VirtualColumnRegistry.visitAllSubExpressions(VirtualColumnRegistry.java:212)
	at org.apache.druid.sql.calcite.rel.DruidQuery.getVirtualColumns(DruidQuery.java:643)

I tried to fix this by using replaceAll, but still saw the exception, so ended up just copying everything to a new collection and going through them that way.


This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

Copy link
Contributor

@cheddar cheddar left a comment

Choose a reason for hiding this comment

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

Making the check mark green.

@clintropolis clintropolis merged commit d30efb1 into apache:master Jul 1, 2022
@clintropolis clintropolis deleted the fix-virtual-column-registry-concurrent-modification branch July 1, 2022 09:24
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
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.

4 participants