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

Create indexed CompareCollectionExpression when left and right parts … #1089

Conversation

debelyoo
Copy link

…are indexed attributes

Purpose

When using an indexed in-memory table, and updating records in it, the performance decreases as the table size increases.
The problem is related to the creation of the CompareCollectionExpression which is used by the update operation on the in-memory table.
The collection executor which is created by parsing the collection expression has an EXHAUSTIVE scope instead of an INDEXED_RESULT_SET scope. And thus, any ‘find’ operation, iterates over all elements of the table.

Goals

Fix #1088

Approach

All details are in Issue #1088

Release note

Use indexed CompareCollectionExpression when left and right parts are indexed attributes

Documentation

N/A

Automation tests

N/A

Security checks

N/A

@suhothayan
Copy link
Contributor

Thanks for your contributions.
Based on the Travis build there are some NullPointers due to your fix, can you please check on it.

@debelyoo
Copy link
Author

debelyoo commented Mar 1, 2019

Ok, it is not as simple as I thought :) I will close this PR and rework on the changes.

@debelyoo debelyoo closed this Mar 1, 2019
@debelyoo
Copy link
Author

debelyoo commented Mar 3, 2019

I limited the use of an indexed CompareCollection expression to the EQUAL operator.
I still have NPE when running the tests... but I'm not sure to understand their root cause.

Let's take test IndexTableTestCase.indexTableTest21(), with siddhi 4.4.0, the method ExhaustiveCollectionExecutor.contains() is called when the first event ({"FOO", 100L}) is sent to checkStoreStream, and it returns false. Which is correct.
With my changes (siddhi 4.4.0 modified), the method CompareCollectionExecutor.contains() is called when the first event ({"FOO", 100L}) is sent to checkStoreStream but raises a NPE.
IMO the call to CompareCollectionExecutor.contains() should also return false, as it does with an ExhaustiveCollection. Am I missing anything ?

@debelyoo debelyoo reopened this Mar 3, 2019
@mohanvive
Copy link
Contributor

@debelyoo we are working on some improvements related to this. We'll look into this with that. If the above changes are work for your use cases then please go ahead.

We'll come with an update soon. Thanks for ur contribution.

@debelyoo
Copy link
Author

debelyoo commented Mar 7, 2019

@mohanvive thanks for your answer. Is there any branch or PR about the work you are doing on this subject ? I would be interested in following your work.

@mohanvive
Copy link
Contributor

@mohanvive thanks for your answer. Is there any branch or PR about the work you are doing on this subject ? I would be interested in following your work.

Sorry for the late @debelyoo . Will update you within today...

@mohanvive
Copy link
Contributor

mohanvive commented Mar 11, 2019

@debelyoo I have spent few hours to identify the root cause for the test failures that encountered with this PR. Then we have figured out issue #1134 is causing the problem/exception. I believe, the issue that you raised in #1088 should also get resolved with the fix that we made in the PR #1135. Can you please try out the test with the fix that we made? Please share the outcome.

BTW, we are interested to get to know more about your work then we could help you through our dev mailing list (https://groups.google.com/forum/#!forum/siddhi-dev) as well. Shall we have a quick call to understand your use cases? That will also help us to improve the capabilities of the Siddhi and define the proper roadmap for the project.

@mohanvive
Copy link
Contributor

@debelyoo Have you got a chance to check this?

@debelyoo
Copy link
Author

@mohanvive Thanks for having looked at it.
I'll try to take a look at it today. I'll keep you posted.

@debelyoo
Copy link
Author

The fix in PR #1135 fixes the performance issue I was experiencing #1088. Thanks a lot !
Do you know when v5.0.0 will be released ?
Regarding our usage of siddhi, I would be glad to have a quick call to discuss with you the use cases we are working on.

@debelyoo debelyoo closed this Mar 13, 2019
@mohanvive
Copy link
Contributor

@debelyoo Actually we are doing some major improvements in the master (5.0.0-SNAPSHOT), it will take few weeks to complete that and do a release. We can do a release in 4.4.X with above fixes and some improvements on Tomorrow. Is that enough for you?

Regarding the call, sure we can have it. I can also update on Siddhi roadmap and plans as well. I am in IST timezone. Can you please suggest a few possible time slots and email address then I could send a calendar invite.

@mohanvive
Copy link
Contributor

The fix in PR #1135 fixes the performance issue I was experiencing #1088. Thanks a lot !
Oh, That's cool.. 😄

@debelyoo
Copy link
Author

@debelyoo Actually we are doing some major improvements in the master (5.0.0-SNAPSHOT), it will take few weeks to complete that and do a release. We can do a release in 4.4.X with above fixes and some improvements on Tomorrow. Is that enough for you?

Yes, that would be fine for me. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants