Add RowGrouping to fix Row Merging on MultiEngine Queries #447

ryankwagner · 2019-04-30T19:44:12Z

No description provided.

patelh · 2019-04-30T22:30:02Z

Dim driven or fact driven?

…ed in OracleQueryGeneratorTest.

ryankwagner · 2019-05-01T18:58:08Z

This is on queries that merge a Druid fact + Oracle Dim query. Currently, RowList addRow uses only a single PrimaryKey alias for merging dim into fact. This causes any multi-value Dimension Rows coming from the Fact (Pricing Type) to collapse into 1 row. The fix is creating a RowGrouping object that contains both the fact-dim join key as its indexAlias, as well as any Fact-side Dimension Columns as "group by" keys to preserve the primary row list.

patelh · 2019-05-01T21:13:26Z

You need to first define the use case which you are solving this for. Multi-engine queries are there for particular use cases. The behavior of multi-engine is dependent on fact driven vs dim driven queries so you need to be certain what you are solving.

patelh · 2019-05-01T21:26:28Z

core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala

+  val dimDrivenPartialRowList: Query => RowList = (q) => {
+    val indexAlias = q.queryContext.indexAliasOption.get
+    val groupByKeys: List[String] = q.queryContext.factGroupByKeys diff List(indexAlias)
+    new DimDrivenPartialRowList(RowGrouping(q.queryContext.indexAliasOption.get, groupByKeys), q)


This breaks the meaning of dim driven queries. Dim driven queries will always have only 1 row for each key from driving dim.

Thanks for the catch, that actually saved me some debugging. :)

patelh · 2019-05-01T21:27:11Z

core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala

+  val dimDrivenFactOrderedPartialRowList: Query => RowList = (q) => {
+    val indexAlias = q.queryContext.indexAliasOption.get
+    val groupByKeys: List[String] = q.queryContext.factGroupByKeys diff List(indexAlias)
+    new DimDrivenFactOrderedPartialRowList(RowGrouping(q.queryContext.indexAliasOption.get, groupByKeys), q)}


Same here, dim driven implies single row per driving dim key.

ryankwagner · 2019-05-02T19:59:24Z

I have updated test cases to reflect the new logic. Right now, the join cases for any query using multiple engines then joining the results is incorrect. This adds columns to group by in order to prevent multivalue Dimension columns in fact from being lost.
The test cases in OracleQueryGenerator shows the fix, but current Query behavior is:
selectFields: ["Ad ID", "Ad Title", "Pricing Type"]
returnedRows from Fact: [1, null, "CPC"], [1, null "CPA"]
returnedRows from Dim: [1, "title", null]
Merged returned rows (current): [1, "title", "CPA"]
That is to say, the first of the two rows gets updated and CPC is lost. The new logic creates a grouping on Pricing Type so that we get 2 returned rows instead of 1, both having the correct information.

patelh · 2019-05-04T00:54:36Z

core/src/test/scala/com/yahoo/maha/core/query/DefaultQueryPipelineFactoryTest.scala

@@ -501,17 +501,23 @@ class DefaultQueryPipelineFactoryTest extends FunSuite with Matchers with Before
        row.addValue("Ad Group ID", 202)
        row.addValue("Impressions", 100)
        row.addValue("Clicks", 1)
+        row.addValue("Pricing Type", "CPC")


Please create separate test for new functionality. Should be easy to copy/paste the parts you need.

core/src/test/scala/com/yahoo/maha/core/query/DefaultQueryPipelineFactoryTest.scala

core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala

…neIssue

ryankwagner · 2019-05-08T18:59:28Z

core/src/test/scala/com/yahoo/maha/core/query/UnionViewRowListTest.scala

@@ -76,6 +76,8 @@ class UnionViewRowListTest extends BaseOracleQueryGeneratorTest with BaseRowList
          Map("Impressions" -> DecType(), "Spend" -> DecType()),
          constAliasToValueMapList = List(Map("Advertiser ID" -> "12345"), Map("Advertiser ID" -> "12345", "Impressions" -> "1.0")))

+    assert(rowList.forall(row=>row.isInstanceOf[Row]))


testing .forall

ryankwagner · 2019-05-08T19:02:57Z

core/src/test/scala/com/yahoo/maha/core/query/oracle/OracleQueryGeneratorTest.scala

@@ -5295,7 +5296,7 @@ class OracleQueryGeneratorTest extends BaseOracleQueryGeneratorTest {
         |            FROM
         |               ( (SELECT  advertiser_id, campaign_id, DECODE(status, 'ON', 'ON', 'OFF') AS "Ad Group Status", id
         |            FROM ad_group_oracle
-         |            WHERE (advertiser_id = 213) AND (id IN (45,39,30,51,48,27,33,54,12,15,42,36,21,18,24,53,41,35,17,50,44,23,38,47,26,11,32,14,20,29,46,52,28,34,55,40,49,43,22,16,37,19,25,31,13))
+         |            WHERE (advertiser_id = 213) AND (id IN (45,34,12,51,19,23,40,15,11,44,33,22,55,26,50,37,13,46,24,35,16,48,21,54,43,32,49,36,39,17,25,14,47,31,53,42,20,27,38,18,30,29,41,52,28))


order changes since rows are now stored in a different order, as rowGrouping modifies mapping positions vs indexAlias key.

Add test to illustrate MultiEngineQuery issue.

c9b9ae7

ryankwagner mentioned this pull request Apr 30, 2019

MultiEngineQuery Collapses Non-Pk Fact Table Dimension Rows #448

Open

Add initial addRow new Functionality with RowGrouping. Test illustrat…

ea57c50

…ed in OracleQueryGeneratorTest.

ryankwagner changed the title ~~Add test to illustrate MultiEngineQuery issue.~~ Add RowGrouping to fix Row Merging on MultiEngine Queries May 1, 2019

patelh reviewed May 1, 2019

View reviewed changes

ryankwagner added 2 commits May 2, 2019 10:50

Fix to fit DimDriven cases, scoverage failing min coverage.

9231e1f

Coverage reached, executors fixed with new RowGrouping logic.

e2154b4

ryankwagner added 2 commits May 3, 2019 07:30

Remove redundant code, clean.

b1a7176

Remove excess row testing & unnecessary row addition.

3a14798

patelh reviewed May 4, 2019

View reviewed changes

core/src/test/scala/com/yahoo/maha/core/query/DefaultQueryPipelineFactoryTest.scala Outdated Show resolved Hide resolved

patelh reviewed May 4, 2019

View reviewed changes

core/src/test/scala/com/yahoo/maha/core/query/DefaultQueryPipelineFactoryTest.scala Outdated Show resolved Hide resolved

patelh reviewed May 4, 2019

View reviewed changes

core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala Outdated Show resolved Hide resolved

ryankwagner added 8 commits May 6, 2019 07:53

Fix multiEngineQuery to only GroupBy Dim Primary Keys.

4d5d4ba

Fix dim driven queries

f9f8d24

Allow for multiple value add to rows

a1075a3

Merge branch 'master' of https://github.com/yahoo/maha into multiEngi…

b090f19

…neIssue

Revert changes pushed into another branch.

3292a87

Revert changes pushed into another branch.

f923a8a

Add fact and Dim RowList tests to demonstrate changes individually.

6a7e6b3

Merge with master & new tests

6a3d7d7

ryankwagner commented May 8, 2019

View reviewed changes

pranavbhole approved these changes May 9, 2019

View reviewed changes

pranavbhole merged commit b7c4298 into yahoo:master May 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RowGrouping to fix Row Merging on MultiEngine Queries #447

Add RowGrouping to fix Row Merging on MultiEngine Queries #447

ryankwagner commented Apr 30, 2019

patelh commented Apr 30, 2019

ryankwagner commented May 1, 2019

patelh commented May 1, 2019

patelh May 1, 2019

ryankwagner May 2, 2019

patelh May 1, 2019

ryankwagner commented May 2, 2019

patelh May 4, 2019

ryankwagner May 8, 2019

ryankwagner May 8, 2019

Add RowGrouping to fix Row Merging on MultiEngine Queries #447

Add RowGrouping to fix Row Merging on MultiEngine Queries #447

Conversation

ryankwagner commented Apr 30, 2019

patelh commented Apr 30, 2019

ryankwagner commented May 1, 2019

patelh commented May 1, 2019

patelh May 1, 2019

Choose a reason for hiding this comment

ryankwagner May 2, 2019

Choose a reason for hiding this comment

patelh May 1, 2019

Choose a reason for hiding this comment

ryankwagner commented May 2, 2019

patelh May 4, 2019

Choose a reason for hiding this comment

ryankwagner May 8, 2019

Choose a reason for hiding this comment

ryankwagner May 8, 2019

Choose a reason for hiding this comment