Resolution using Table Context #2004
Conversation
| String value = matcher.group(1); | ||
| if (!value.startsWith("$$") && !value.startsWith("sql ")) { | ||
| references.add(value); | ||
| } |
There was a problem hiding this comment.
Added this temporarily until ColumnVisitor is removed for test cases to work for SQL helper.
rishi-aga
force-pushed
the
populateTableContext
branch
from
fcab235
to
ff753ef
Compare
|
|
||
| private final Handlebars handlebars = new Handlebars() | ||
| .with(EscapingStrategy.NOOP) | ||
| .registerHelper("sql", new Helper<Object>() { |
There was a problem hiding this comment.
Does this Helper return Object or String?
| } | ||
| } | ||
|
|
||
| private static void parseArguments(String argsString, Map<String, Object> pinnedArgs) |
There was a problem hiding this comment.
This code is common with RSQLFilterDialect. Maybe we should migrate it to another shared utility class?
| String invokedColumnExpr = table.getColumnMap().get(invokedColumnName).getExpression(); | ||
|
|
||
| Map<String, Object> currentColumnArgs = (Map<String, Object>) | ||
| ((Map<String, Object>) currentTableCtx.get(COL_PREFIX)) |
There was a problem hiding this comment.
Double cast doesn't feel necessary here.
| Table table = metaDataStore.getTable(queryable.getName(), queryable.getVersion()); | ||
|
|
||
| // Add the default argument values stored in metadata store. | ||
| if (table != null) { |
There was a problem hiding this comment.
Do we ever expect table to be null?
There was a problem hiding this comment.
It can be null when it runs for Nested Queries.
|
|
||
| joinQueryable.getJoins().forEach((name, join) -> { | ||
| SQLTable joinTable = metaDataStore.getTable(join.getJoinTableType()); | ||
| newCtx.addJoin(name, joinTable); |
There was a problem hiding this comment.
Instead of storing the joins in the context, we can fetch them directly from the joinTable.
| return resolveHandlebars(invokedTableCtx, invokedColumnName, invokedColumnExpr, pinnedArgs); | ||
| } | ||
|
|
||
| public void addJoin(String joinName, Queryable joinQueryable) { |
There was a problem hiding this comment.
We can remove this function if we leverage the joins stored in the associated Queryable.
| @@ -87,7 +87,8 @@ public SQLMetricProjection(Metric metric, | |||
| @Override | |||
| public String toSQL(Queryable source, SQLReferenceTable lookupTable) { | |||
| if (expression.matches(".*\\{\\{.*\\}\\}.*")) { | |||
| return lookupTable.getResolvedReference(source, getName()); | |||
| //TODO: Add query context to Table Context before calling get | |||
There was a problem hiding this comment.
Thinking to change this method to public String toSQL(Queryable source, SQLReferenceTable lookupTable, Map<String, Object> queryContext) { and then pass user provided queryContext also for resolving definitions.
| import javax.persistence.Id; | ||
| import javax.sql.DataSource; | ||
|
|
||
| public class TableContextTest { |
There was a problem hiding this comment.
Not sure if we want to do this as part of this PR or another - but we also need to test
- Join template substitution
- FromQuery template substitution.
There was a problem hiding this comment.
I will submit PRs for these against this PR.
There was a problem hiding this comment.
We also need to test nested queries. I don't see any tests for those.
|
|
||
| // default value of 'format' argument is used for 'conversion_rate' column | ||
| assertEquals("TO_CHAR(SUM(`com_yahoo_elide_datastores_aggregation_metadata_RevenueFact`.`revenue`) * " | ||
| + "TO_CHAR(`com_yahoo_elide_datastores_aggregation_metadata_RevenueFact_rate`.`conversion_rate`, 999D00), " |
There was a problem hiding this comment.
I don't understand how the conversion rate format here is different than the test below.
There was a problem hiding this comment.
definition of revenue column uses {{rate.conversionRate}} and revenueUsingSqlHelper uses {{sql from='rate' column='conversionRate'}}
For first scenario, its using default value of 'format' argument defined for conversionRate field in rate table.
In the case of SQL helper, its using default value of 'format' argument defined for revenueUsingSqlHelper field
rishi-aga
force-pushed
the
populateTableContext
branch
3 times, most recently
from
520a255
to
f3ac673
Compare
|
|
||
| if (!isEmpty(argsString)) { | ||
|
|
||
| Matcher matcher = FILTER_ARGUMENTS_PATTERN.matcher(argsString); |
There was a problem hiding this comment.
We should move the FILTER_ARGUMENTS_PATTERN to this file. Does this function make sense in TypeHelper?
rishi-aga
force-pushed
the
populateTableContext
branch
from
f3ac673
to
4ba41d1
Compare
| @@ -80,6 +84,13 @@ public Object get(Object key) { | |||
| newCtx.put(column.getName(), column.getExpression()); | |||
There was a problem hiding this comment.
Maybe this (copy of queryable columns into the context) should happen in the constructor of the TableContext? Or maybe they shoudln't be stored in the map at all (and just referenced directly from the queryable like the joins)?
There was a problem hiding this comment.
Removed them from the Map.
| @@ -92,7 +103,7 @@ public Object get(Object key) { | |||
| verifyKeyExists(key, super.keySet()); | |||
|
|
|||
| Object value = super.get(key); | |||
| if (key.toString().startsWith("$$")) { | |||
| if (value instanceof Map) { | |||
There was a problem hiding this comment.
Instead of checking types here, I wonder if we just look to see if the key is one of the queryable columns - then call resolveHandlebars on the value. Otherwise, just return what is in the map.
| import javax.persistence.Id; | ||
| import javax.sql.DataSource; | ||
|
|
||
| public class TableContextTest { |
There was a problem hiding this comment.
We also need to test nested queries. I don't see any tests for those.
rishi-aga
force-pushed
the
populateTableContext
branch
from
6e4a579
to
9a27be4
Compare
One of the PR that Resolves #1829
Description
Expression resolution using TableContext.
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.