Skip to content

Javascript filter with multiple columns#2210

Closed
sirpkt wants to merge 12 commits intoapache:masterfrom
sirpkt:javascript-filter-with-multiple-columns
Closed

Javascript filter with multiple columns#2210
sirpkt wants to merge 12 commits intoapache:masterfrom
sirpkt:javascript-filter-with-multiple-columns

Conversation

@sirpkt
Copy link
Copy Markdown
Contributor

@sirpkt sirpkt commented Jan 6, 2016

This PR fixes #2187

It is implemented like nested loop self-join.
So, if the number of columns increases, it would take much time.
For backward compatibility, usage of javascript filter with one column is maintained.

Currently, complement() of roaring bitmap does not work correctly yet,
test code with NotFilter is commented temporarily.

@sirpkt sirpkt closed this Jan 6, 2016
@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 6, 2016

I found I made some mistakes on my test code
so I will reopen after fix the test code

@sirpkt sirpkt reopened this Jan 6, 2016
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

xor should take care of these two checks

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Jan 6, 2016

@sirpkt complement() in roaring bitmap has been fixed in #2198, so you should rebase your changes on latest master and uncomment your test.

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 7, 2016

I found that complement bug is not fixed completely with #2198 so made another issue #2218

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 7, 2016

I think #2217 looks more general so it is better to close this PR.
However, @navis, if you do not mind, how about to include my test cases in #2217 ?

@sirpkt sirpkt closed this Jan 7, 2016
@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 11, 2016

As @navis mentioned in #2217 , we decided to keep both patch as each has its own advantage in different cases.
So, I reopen the PR.

@sirpkt sirpkt reopened this Jan 11, 2016
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about to make two constructor? But his will need opinions from others because I don't know well on guava.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Guice, not guava, I mean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm also not familiar with Jackson,
but, I experienced the problem at deserialization of Json string when multiple constructors are used.

@navis
Copy link
Copy Markdown
Contributor

navis commented Jan 12, 2016

Generally, it's implemented in accordant with current implementation but cartesian is still looks painful. I think we should reorder filters and merge bitmap from simple filter with one from complex like this to minimize the pain (when AND filter is applied). But it can be handled in following issue.

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.

I would like JavaScript filters to be able to work on multiple columns

5 participants