{"action":"created","issue":{"url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/239","labels_url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/239/labels{/name}","comments_url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/239/comments","events_url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/239/events","html_url":"https://github.com/afawcett/declarative-lookup-rollup-summaries/issues/239","id":103835703,"number":239,"title":"Concatenation Rollups order maybe none Deterministic - Consider moving order by from LREngine.RollupSummaryField to LREngine.Context","user":{"login":"jondavis9898","id":12914673,"avatar_url":"https://avatars.githubusercontent.com/u/12914673?v=3","gravatar_id":"","url":"https://api.github.com/users/jondavis9898","html_url":"https://github.com/jondavis9898","followers_url":"https://api.github.com/users/jondavis9898/followers","following_url":"https://api.github.com/users/jondavis9898/following{/other_user}","gists_url":"https://api.github.com/users/jondavis9898/gists{/gist_id}","starred_url":"https://api.github.com/users/jondavis9898/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jondavis9898/subscriptions","organizations_url":"https://api.github.com/users/jondavis9898/orgs","repos_url":"https://api.github.com/users/jondavis9898/repos","events_url":"https://api.github.com/users/jondavis9898/events{/privacy}","received_events_url":"https://api.github.com/users/jondavis9898/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":21,"created_at":"2015-08-29T07:34:01Z","updated_at":"2015-10-17T20:52:58Z","closed_at":null,"body":"Currently, order by clauses are handled by RollupSummaryFields in a \"Then By\" approach.  However, within DLRS as of PR #223, there would never be a situation where any two RollupSummaryFields within the same context would have a different order by clause except for when the Lookup Rollup Summary itself does not contain an order by field/clause.\r\n\r\nThe documentation & help text for Order By field states that when no order by is specified, it will default to the FieldToAggregate.  However, there are cases where this behavior does not occur and is a result of the \"Then By\" approach on a shared context.  For example, if two rollups are defined and neither specify an order by, the resulting SOQL will have an order by of \"FieldToAggregate1, FieldToAggregate2.\"  This results in the second rollup not being ordered by it's field to aggregate as the documentation states it will be.  For an example, see https://github.com/jondavis9898/declarative-lookup-rollup-summaries/commit/84caee2c93f11b5c10c2cf1cacbe3596661facb9 and the comments by the asserts on https://github.com/jondavis9898/declarative-lookup-rollup-summaries/commit/84caee2c93f11b5c10c2cf1cacbe3596661facb9#diff-a59e8228c780cd01c3372282a42d4712R868, https://github.com/jondavis9898/declarative-lookup-rollup-summaries/commit/84caee2c93f11b5c10c2cf1cacbe3596661facb9#diff-a59e8228c780cd01c3372282a42d4712R874 and https://github.com/jondavis9898/declarative-lookup-rollup-summaries/commit/84caee2c93f11b5c10c2cf1cacbe3596661facb9#diff-a59e8228c780cd01c3372282a42d4712R880.\r\n\r\nIn the case where multiple rollups each contain the same specific order by, they will share a context.  Currently, within LREngine there is logic that tracks fields included in order by and discards duplicates (https://github.com/afawcett/declarative-lookup-rollup-summaries/blob/master/rolluptool/src/classes/LREngine.cls#L168).  Since a context in this situation will never have different order by fields, this processing is unnecessary.\r\n\r\nIn reviewing the LRE project itself, it appears that the order by enhancements within DLRS have not made it in to that project.  Therefore, if we make changes within DLRS, I believe it would only effect DLRS internally and anyone that uses DLRS source instead of DLRS package.\r\n\r\nGiven the above, I have the following recommendations and would appreciate feedback/thoughts:\r\n\r\n1) Move the \"order by\" to LREngine.Context and handle in the same manner that relationship criteria (where clause) is handled.  Each context has a unique order by and all rollups within the context subscribe to that order by.  Making this change would eliminate the extra tracking/processing to detect duplicates, etc. thereby increasing performance of the package.\r\n\r\n2) When no order by is specified, do not generate any order by for the context.  This results in a non-deterministic query that is not effected by any rollup fields included in the context.  While the current approach is non-deterministic, it leads to undesirable side effects such as described above with \"default ordering.\"  This change could also include not including the lookup field in the order by clause (https://github.com/afawcett/declarative-lookup-rollup-summaries/blob/master/rolluptool/src/classes/LREngine.cls#L131) as the only reason this is included currently is to assist in the \"merging\" of master record id's during the processing of rollup queryResults.  ** Edit ** After some additional thinking on this last point, I think it might make sense to leave the lookup field in the order by as it won't effect any ordering outcome and it is likely better performing to order in DB and then loop checking for a different master id (like the code currently does) then continually lookup in the Map on each iteration through queryresults.\r\n\r\n3) Update the documentation to remove the mention of a rollup defaulting to the fieldtoaggregate when no order by is specified.\r\n\r\nAppreciate the consideration and any thoughts/feedback.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/comments/148950319","html_url":"https://github.com/afawcett/declarative-lookup-rollup-summaries/issues/239#issuecomment-148950319","issue_url":"https://api.github.com/repos/afawcett/declarative-lookup-rollup-summaries/issues/239","id":148950319,"user":{"login":"afawcett","id":1167760,"avatar_url":"https://avatars.githubusercontent.com/u/1167760?v=3","gravatar_id":"","url":"https://api.github.com/users/afawcett","html_url":"https://github.com/afawcett","followers_url":"https://api.github.com/users/afawcett/followers","following_url":"https://api.github.com/users/afawcett/following{/other_user}","gists_url":"https://api.github.com/users/afawcett/gists{/gist_id}","starred_url":"https://api.github.com/users/afawcett/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/afawcett/subscriptions","organizations_url":"https://api.github.com/users/afawcett/orgs","repos_url":"https://api.github.com/users/afawcett/repos","events_url":"https://api.github.com/users/afawcett/events{/privacy}","received_events_url":"https://api.github.com/users/afawcett/received_events","type":"User","site_admin":false},"created_at":"2015-10-17T20:52:58Z","updated_at":"2015-10-17T20:52:58Z","body":"Yep, will look on Sunday, thanks for your patience. "}}