Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Union in Decoupled planning #17354
base: master
Are you sure you want to change the base?
Support Union in Decoupled planning #17354
Changes from 148 commits
1792656
be298b7
726eec6
fadaa32
6daa7d4
1dc39ea
87fc449
d8f440d
df44cb5
8f9b808
932d61d
d62e993
d5ecc0c
4f382e5
44ea85e
24ae912
6035164
9ef426f
2f78fd3
cae132a
63caf06
dc25d2f
60d0408
43ae60d
8f58d49
2a4ab81
f96969d
b36e9eb
27c80e8
5284ad8
ec5a350
b5471e0
94093b3
ad00767
4dd587e
fa4e598
870de00
e346500
3644b16
b8138b8
763e1a0
fc8a1f1
dcab034
98034b1
7fc3331
8fe0331
8628e6d
bc4c881
b3c8ceb
02b126c
451d29d
523d82d
13abfb2
2503974
ae21921
5d2f088
ae40a8b
ab03132
bba4f57
95aa746
d4055ac
8786707
00129b2
b5063db
84545f5
25fa778
949118f
b59771c
01cc4f5
362bd18
94ece7a
078f7c9
d0cbaf5
031f9de
d38aa5e
8b1d3a1
55d87d8
2fb6794
6533e61
a1d8748
787a332
c78c935
0d99b61
874438f
29756bc
97c3749
f9ff96c
5e6eda1
22bdd3c
0f36ec6
4eae978
52d5fda
4a2c095
e6ae62b
ede7c68
b9e4b93
bd7eacc
0bc2635
cedc6de
a57fae8
a05e363
89e06b2
e7debd2
b47d92b
eebb276
6e47463
68a7a70
1b1bbda
150477e
c3e6ffe
64853c4
8e8a2e3
371f863
5b18b8a
9c1e2a1
15d1b25
6bf5cb9
5327b1c
9aaefc4
f45787a
eb5666e
ff5aadb
518a455
2700444
1b22d22
b494861
5bc29e3
1f7bd09
d605166
19583f8
835fcb4
33aa8c3
e2aa507
113f13d
b27511f
8c08649
574b337
c17a3ef
4a76077
15b6987
51bbf55
a3f8445
069e700
66736dd
98cb67a
7fbc842
ab21c40
1bdcc4f
863b947
61ab951
633adc1
de4c2df
f4d16a6
d4c9877
862493e
689b5b5
3cd56c9
43c1078
515d064
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming issue: "QueryExecutor" is extremely generic, the name should more reflect where in the lifecycle the query is being executed, so that it's more clear what this is in charge of. We should also have javadoc describing what this is responsible for. I'd suggest something like,
QueryEntryPointRunner
or something similar to that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we name the interface
QueryLogic
and have a method calledentryPoint
. This would actually align with something else I'm doing as well, so I kinda like that. Especially as the distinction between the ToolChest and the QueryRunnerFactory has basically disappeared as the system has evolved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods for
getDataSources
andwithDataSources
I would hope that we can eliminate on the interface. They are a leaky abstraction, I think that any code that would actually need these would be able to be avoided by implementing theQueryExecutor
thingie which means that these methods don't need to exist on the interface anymore, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were needed for 2 reasons:
DataSource
class alredy had [get|with]Children methods ; since this changes the theQuery
to have multiple of them - it feeled like communicating it clearly is better - so that all existing places itrecursivelyClearContext
generateSubqueryIds
andinsertSubqueryIds
use these to set context variablesalternative options could be:
QueryDataSource
(so that it does not appear on the mainQuery
interface)I've done the second approach from above; I'm not sure about the complexity that will cause in the long run but its not that bad right now...I had to add 3 instanceof-s:
QueryDataSource
clearRecursivelyContext
methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this comment elsewhere as well, but what do you think about calling this
QueryLogic
and then the method you are creating isinitialEntryPoint()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this comment elsewhere, but I don't think we need the Warehouse interface really and should just expand what the Conglomerate can do.