[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the issue:

https://github.com/apache/drill/pull/978
  
+1, LGTM.


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[sohami]

Github user sohami commented on the issue:

https://github.com/apache/drill/pull/978
  
+1 LGTM. 
Thanks for clarification.


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[paul-rogers]

Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/978
  
Rebased and squashed commits.


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[paul-rogers]

Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/978
  
@sohami, pushed a commit that addresses your PR comments.

Regarding the "one stop shop" comment. This PR removes the 
`OperExecContextImpl` class. That class was an earlier attempt to combine the 
three items into one. Additional experience showed that the existing operator 
context could do that task instead.

This PR did not change existing operators to avoid passing around multiple 
items. Instead, it allows new code (for the batch size limitation project) to 
pass just the operator context, and use that to obtain the other two items. The 
external sort had to be changed because it used the old `OperExecContextImpl` 
for that purpose, and so the code had to be updated when `OperExecContextImpl` 
was removed.


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[sohami]

Github user sohami commented on the issue:

https://github.com/apache/drill/pull/978
  
Looking into the PR and based on description it says below:

_A result of these changes is that OperatorContext is now a one-stop-shop 
for services needed by operators. It now provides:_

- Access to the fragment context: getFragmentContext()
- Access to the physical operator definition (AKA “physical operator”): 
getOperatorDefn().

_Because of these changes, we need no longer pass around the triple of 
fragment context, operator definition and operator context — something needed 
by the “managed” sort and upcoming PRs._

Based on my understanding it looks like existing interface and 
implementation `OperExecContextImpl` do provide access to `FragmentContext` and 
`PopConfig` (operator Defn). The instance of this context is created inside 
`ExternalSortBatch` which has access to FragmentContext and PopConfig already. 
Also `SortImpl` only takes in `OperExecContext`. I am little confused how the 
change avoided the need to pass around the triplet ?


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[paul-rogers]

Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/978
  
@jinfengni, sorry my description was a bit ambiguous. This PR has two goals:

1. Clean up the contexts based on what has been learned recently.
2. Evolve the contexts to allow "sub-operator" unit testing without mocks.

Drill has many tests that use mocks and these are unaffected by the change. 
The PR simply allows new tests to do sub-operator testing without mocks, when 
convenient. 


---

[GitHub] drill issue #978: DRILL-5842: Refactor and simplify the fragment, operator c...

[priteshm]

Github user priteshm commented on the issue:

https://github.com/apache/drill/pull/978
  
@sohami  can you please review this?


---