Lineage lookups #3151
Lineage lookups #3151
Conversation
…_lookup_prototype
invalidateMaterialsForRun()
labkey-susanh
left a comment
labkey-susanh
left a comment
There was a problem hiding this comment.
Code seems reasonable enough, but there's something wrong in here because the Data Class columns are not added as lookups and display only the rowId values.
|
|
||
|
|
||
| /* Do incremental update to existing cached data. There is no provision for deleting rows. */ | ||
| public void upsert() |
There was a problem hiding this comment.
Is this method needed?
There was a problem hiding this comment.
used by ClosureQueryHelper.incrementalRecompute(). MQH doesn't directly expose the names of names of the tables it's managing, so this seemed right.
| String[] cteParts = StringUtils.splitByWholeSeparator(cte,"/*FROM*/"); | ||
| assert cteParts.length == 2; | ||
|
|
||
| return new SQLFragment(cteParts[0]).append(from).append(cteParts[1]).append(select); |
There was a problem hiding this comment.
Seems like you might need some spaces around the from here.
There was a problem hiding this comment.
This matches the text in the SQL string constants above.
There was a problem hiding this comment.
On line 146, it looks like this:
.append(cteParts[0]).append(" ").append(from).append(" ").append(cteParts[1])
So, either the extra space appends aren't needed above, or they would be needed here, or I'm not reading this correctly. Not a big deal if they're extra above, but I get bitten by these missing spaces too many times.
experiment/src/org/labkey/experiment/api/ClosureQueryHelper.java
Outdated
Show resolved
Hide resolved
experiment/src/org/labkey/experiment/api/ClosureQueryHelper.java
Outdated
Show resolved
Hide resolved
experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java
Outdated
Show resolved
Hide resolved
…_lookup_prototype
…_lookup_prototype
placeholder name for lookup column
labkey-susanh
left a comment
labkey-susanh
left a comment
There was a problem hiding this comment.
Definitely working better now for data classes. I've not tested extensively, though.
|
|
||
| if (null != _ss && AppProps.getInstance().isExperimentalFeatureEnabled(ExperimentModule.EXPERIMENTAL_LINEAGE_PARENT_LOOKUP)) | ||
| { | ||
| MutableColumnInfo lineageLookup = ClosureQueryHelper.createLineageLookupColumnInfo("Ancestor Lookups Placeholder", this, _rootTable.getColumn("rowid"), _ss); |
There was a problem hiding this comment.
Could just call this "Ancestors" or "Ancestor Lookups".
There was a problem hiding this comment.
Or "Lineage Lookups". I figured the spec was the right place to make a decision, but that if I put a "reasonable" thing in the code it would be sticky.
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
This is great. A little different than what I understood from the spec, but matches more my understanding of the use case.
Seems like this could help with Sample Finder resolving lookups as well?
A couple inline comments to think about.
| """, MAX_LINEAGE_LOOKUP_DEPTH); | ||
|
|
||
| static String pgMaterialClosureSql = """ | ||
| SELECT Start_, CASE WHEN COUNT(*) = 1 THEN MIN(rowId) ELSE -1 * COUNT(*) END AS rowId, targetId |
There was a problem hiding this comment.
So this gives a count of rows when not 1:1. I imagine in some cases we'll want to provide some feedback why a value isn't showing up in a column that is not 1:1. Any thoughts how to propagate that up in the query? Looks like this will just be part of the join and won't match anything.
There was a problem hiding this comment.
Looks like the negative row count shows up in columns that are just selecting the material or dataclass instead of a value in the material or dataclass. Might be useful for providing user feedback in UIs, but if not providing an error in the actual response, maybe at least in a future fb provide a comment or explanation for anyone looking to use this
|
|
||
| enum TableType | ||
| { | ||
| SampleType("Materials", SchemaKey.fromParts("exp","materials") ) |
There was a problem hiding this comment.
Might be nice if we could alias these eventually (samples, sources, registry,...), especially if we're going to make these available in-app eventually.
Rationale
Support for filter/sort on columns in parent sample/dataclass
Under experimental feature flag for the moment.
Related Pull Requests
Changes