[2/4] Offline pipeline evaluation and tests #8908
Conversation
|
Vertex AI Mock Responses Check
|
| */ | ||
| public _userDataWriter: AbstractUserDataWriter, | ||
| readonly stages: Stage[], | ||
| readonly converter: unknown = {} |
There was a problem hiding this comment.
not critical, but we've been removing converter from the pipeline constructors.
| import { JsonProtoSerializer } from '../remote/serializer'; | ||
|
|
||
| /** | ||
| * Base-class implementation |
There was a problem hiding this comment.
We should update this comment before the release
| * @param stages | ||
| * @param converter | ||
| */ | ||
| newPipeline( |
There was a problem hiding this comment.
We can keep this pattern in RealtimePipeline, but it's not required. It was useful in Pipeline / LitePipeline. Could be replaced with a direct call to new RealtimePipeline() for simpler code
| new Sort( | ||
| this.readUserData( | ||
| 'sort', | ||
| this.readUserData('sort', optionsOrOrderings.orderings) |
There was a problem hiding this comment.
This might be a redundant call to readUserData
| AggregateFunction, | ||
| ListOfExprs, | ||
| isNan, | ||
| isError |
There was a problem hiding this comment.
I'm assuming these linter errors are false positives caused by the way the code was split. Either way, I'm sure we will have some formatting and linter fixes when merging to main.
| } else if (functionExpr.name === 'timestamp_sub') { | ||
| return new CoreTimestampSub(functionExpr); | ||
| } | ||
| } else if (expr.exprType === 'AggregateFunction') { |
There was a problem hiding this comment.
Does this still work since AggregateFunction no longer extends Expr?
| return EvaluateResult.newError(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This pattern seems to be repeated often. I wonder if we can refactor this into some shared code to reduce code size
There was a problem hiding this comment.
let result = {};
if (evaluateParams(expr.params, [['BOOLEAN']], result) {
// not successful
return result.errorResult; // either newError or newNull
}
// use evaluated params
result.evaluated[0]
| } | ||
|
|
||
| if (foundNullAtLeastOnce) { | ||
| return EvaluateResult.newNull(); |
There was a problem hiding this comment.
Seems like you will not hit this case, the loop would have already returned
| } | ||
| case 'NULL': { | ||
| foundNull = true; | ||
| foundNullAtLeastOnce = true; |
There was a problem hiding this comment.
These foundNull(.*) seem to be redundant
| stringValue: evaluated.value?.stringValue | ||
| ?.split('') | ||
| .reverse() | ||
| .join('') |
There was a problem hiding this comment.
We should mark this as an implementation to evaluate efficiency, if you have not already. On the surface, this appears like it would be slow.
| switch (evaluated.type) { | ||
| case 'NULL': | ||
| return EvaluateResult.newNull(); | ||
| case 'STRING': { |
There was a problem hiding this comment.
Reverse and some other functions support Bytes in addition to string. Are bytes encoded as string somewhere upstream of this call, or will we need to add support for bytes?
wu-hui
force-pushed
the
wuandy/RealPpl_2
branch
from
6710057 to
0c570ab
Compare
| @@ -242,41 +288,322 @@ export function toPipeline(query: Query, db: Firestore): Pipeline { | |||
| } | |||
| } | |||
|
|
|||
| return pipeline; | |||
| return pipeline.stages; | |||
| } | |||
|
|
|||
| function whereConditionsFromCursor( | |||
There was a problem hiding this comment.
It probably got lost in the merge. But the implementation in feat/pipelines is more performant and easier to read. Consider using that implementation instead.
| hasOrder = true; | ||
| // add exists to force sparse semantics | ||
| // Is this really needed? | ||
| // newStages.push(new Where(new And(stage.orders.map(order => order.expr.exists())))); |
There was a problem hiding this comment.
Does sparse semantics need to be enforced? If not, should we remove this commented out code?
| 'Empty document paths are not allowed in DocumentsSource' | ||
| ); | ||
| } | ||
| if (stage.docPaths) { |
There was a problem hiding this comment.
stage.docPaths will always be truthy since we referenced stage.docPaths.length above
| stage: DocumentsSource, | ||
| input: PipelineInputOutput[] | ||
| ): PipelineInputOutput[] { | ||
| if (stage.docPaths.length === 0) { |
There was a problem hiding this comment.
if (stage.docPaths && stage.docPaths.length === 0) {
There was a problem hiding this comment.
Consider this if ther is ever a condition where docPaths is unset
| { serializer: pipeline.serializer }, | ||
| d2 as MutableDocument | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Would be nice if we could memoize leftValue and rightValue. May be something for a future PR.
| * @param value The value of the constant. | ||
| */ | ||
| constructor(private value: unknown) { | ||
| constructor( |
There was a problem hiding this comment.
I think at a minimum we want @hideconstructor
| export function currentContext(): FunctionExpr { | ||
| return new FunctionExpr('current_context', []); | ||
| } | ||
|
|
There was a problem hiding this comment.
We should delete this expression. It will not be exposed in the SDKs. This may have been added back in a merge.
| * // Check if the document has a field named "phoneNumber" | ||
| * exists("phoneNumber"); | ||
| * // Check if the result of a calculation is NaN | ||
| * isNaN("value"); |
There was a problem hiding this comment.
It appears as if the comments for this and the overload above were reverted in merge. The comments should apply to exists not isNaN
| * | ||
| * ```typescript | ||
| * // Check if the result of a calculation is NaN | ||
| * isNaN(field("value").divide(0)); | ||
| * ``` | ||
| * | ||
| * @param value The expression to check. | ||
| * @return A new {@code Expr} representing the 'isNaN' check. | ||
| * @return A new {@code Expr} representing the 'isNull' check. |
| * // Check if the result of a calculation is NaN | ||
| * isNaN("value"); | ||
| * // Check if the result of a calculation is null. | ||
| * isNull("value"); |
| return new CorePipeline(pipeline.serializer, newStages); | ||
| } | ||
|
|
||
| export type QueryOrPipeline = Query | CorePipeline; |
There was a problem hiding this comment.
Because QueryOrPipeline is a union type that is used throughout the implementation, to support porting to other platforms, we should change this to a common interface between the two types.
| return queryEquals(left as Query, right as Query); | ||
| } | ||
|
|
||
| export type TargetOrPipeline = Target | CorePipeline; |
There was a problem hiding this comment.
Question to be covered with the team. But what allows us to skip creating of an equivalent "Pipeline Target" class
| stage.orders.some( | ||
| order => | ||
| order.expr instanceof Field && | ||
| order.expr.fieldName() === DOCUMENT_KEY_NAME |
There was a problem hiding this comment.
Does NAME guarantee stable sort on a collection group?
There was a problem hiding this comment.
Yeah, I guess it will. This is the full path not just the ID.
wu-hui
force-pushed
the
wuandy/RealPpl_2
branch
from
0c570ab to
6f0e635
Compare
No description provided.