Skip to content

Comments

Add missing Functionality in MaterializedViewQueryToolChest#9445

Closed
ritik02 wants to merge 1 commit intoapache:masterfrom
ritik02:materialized-view
Closed

Add missing Functionality in MaterializedViewQueryToolChest#9445
ritik02 wants to merge 1 commit intoapache:masterfrom
ritik02:materialized-view

Conversation

@ritik02
Copy link

@ritik02 ritik02 commented Mar 2, 2020

Add postMergeQueryDecoration() for TopN Queries and decorateObjectMapper() for GroupBy Queries in MaterializedViewQueryToolChest .

Description

  1. MaterializedViewQueryToolChest did not implement postMergeQueryDecoration() for TopN Queries hence not returning expected results . As without postMergeQueryDecoration() , each segment returns only topN results instead of max(topN,1000) hence after merging , results are not as expected.
  2. MaterializedViewQueryToolChest did not implement decorateObjectMapper() for GroupBy Queries hence returning ArrayBasedResults whereas expected is MapBasedResults.

This PR has:

  • been self-reviewed.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • been tested in a test Druid cluster.

Key changed/added classes in this PR
  • MaterializedViewQueryQueryToolChest
  • MaterializedViewQueryQueryToolChestTest

@ritik02 ritik02 changed the title dd postMergeQueryDecoration() in MaterializedViewQueryToolChest for TopNQuery and decorateObjectMapper() in MaterializedViewQueryToolChest for GroupByQuery Add postMergeQueryDecoration() ,decorateObjectMapper() in MaterializedViewQueryToolChest Mar 2, 2020
@ritik02 ritik02 changed the title Add postMergeQueryDecoration() ,decorateObjectMapper() in MaterializedViewQueryToolChest Add missing Functionality in MaterializedViewQueryToolChest Mar 2, 2020
@ritik02 ritik02 force-pushed the materialized-view branch from d27462e to 429629d Compare March 2, 2020 12:06
@ritik02 ritik02 force-pushed the materialized-view branch from 429629d to f8edd0d Compare March 2, 2020 13:13
@stale
Copy link

stale bot commented May 1, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label May 1, 2020
@ritik02
Copy link
Author

ritik02 commented May 18, 2020

@jihoonson

@stale
Copy link

stale bot commented May 18, 2020

This issue is no longer marked as stale.

@stale stale bot removed the stale label May 18, 2020
@ritik02
Copy link
Author

ritik02 commented May 18, 2020

@zhangxinyu1

@stale
Copy link

stale bot commented Jul 18, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Jul 18, 2020
@stale
Copy link

stale bot commented Aug 16, 2020

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale bot closed this Aug 16, 2020
@ritik02
Copy link
Author

ritik02 commented Nov 19, 2020

Can anyone look into this pull request . Materialized views are being used by a lot people and it gives incorrect query results , ive resolved this .

@abhishekagarwal87
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants