Conversation
…dk into markduckworth/ppl
…dk into markduckworth/ppl
…ase-js-sdk into markduckworth/ppl-augment
…ase-js-sdk into markduckworth/ppl-augment
…eport steps of the build to complete
…ts in protos.json
Updated strictValueEquals to return 'EQ' for (null, null). Introduced 'TYPE_MISMATCH' in Equality type and handled it in array and map comparisons to return 'NOT_EQ'. Reverted null short-circuits in ordering comparisons to return FALSE for cross-type comparisons. Updated expectations in numerous tests to align with backend behavior. Fixed test bug in null_semantics.test.ts. Disabled failing spec test for pipelines.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new RealtimePipeline class and associated infrastructure to support complex data transformation and query pipelines in Firestore, including real-time capabilities. It adds necessary types, serialization logic, and integration tests. My review identified several concerns: a temporary circular dependency check bypass and commented-out property mangling in Rollup configuration that should be addressed, a potential issue with sort stage invalidation in the pipeline engine, a duplicate dependency in package.json, a typo in a documentation comment, and commented-out code in a test file that should be properly skipped or removed.
| // TODO reenable. This is a temp workaround to allow build | ||
| //throw new Error(warning); |
There was a problem hiding this comment.
Disabling the circular dependency check seems to be a temporary workaround. While understandable for a large refactoring, it's important to ensure this is re-enabled before merging to prevent introducing new circular dependencies.
| // renameInternals(service.getProgram(), { | ||
| // publicIdentifiers, | ||
| // prefix: '__PRIVATE_' | ||
| // }) |
There was a problem hiding this comment.
The renameInternals transformer is commented out. This will prevent mangling of private properties, potentially increasing bundle size and exposing internal APIs. Is this change intentional for this PR, or should it be reverted before merging?
| // return the last sort stage, throws exception if it doesn't exist | ||
| // TODO(pipeline): this implementation is wrong, there are stages that can invalidate | ||
| // the orderings later. The proper way to manipulate the pipeline so that last Sort |
There was a problem hiding this comment.
The TODO here correctly identifies that this implementation is not robust. Simply finding the last Sort stage is insufficient, as subsequent stages like Aggregate or Distinct could invalidate the ordering. This might lead to incorrect results for some pipelines.
| "prettier": "2.8.8", | ||
| "protobufjs-cli": "^1.1.3", | ||
| "protractor": "5.4.2", | ||
| "protobufjs-cli": "^1.1.3", |
There was a problem hiding this comment.
This line introduces a duplicate protobufjs-cli dependency in devDependencies. Please remove one of the entries to keep the dependencies clean.
| import { PipelineSource, Pipeline, RealtimePipeline } from '../dist/pipelines'; | ||
|
|
||
| // Augment the Firestore and Query classes with the pipeline() method. | ||
| // Augument the Firestore and Query classes with the pipeline() method. |
There was a problem hiding this comment.
There's a typo here.
| // Augument the Firestore and Query classes with the pipeline() method. | |
| // Augment the Firestore and Query classes with the pipeline() method. |
| // const accumulator = new EventsAccumulator<DocumentSnapshot>(); | ||
| // const unsubscribe = onSnapshot(doc, accumulator.storeEvent); | ||
| // | ||
| // // Wait for watch to initialize and deliver first event. | ||
| // await accumulator.awaitRemoteEvent(); | ||
| // | ||
| // // Use a transaction to perform a write without triggering any local events. | ||
| // await runTransaction(db, async txn => { | ||
| // txn.set(doc, { value: 'done' }); | ||
| // }); | ||
| // | ||
| // // Wait to see the write on the watch stream. | ||
| // const docSnap = await accumulator.awaitRemoteEvent(); | ||
| // expect(docSnap.get('value')).to.equal('done'); | ||
| // | ||
| // unsubscribe(); | ||
| }); |
There was a problem hiding this comment.
The entire body of this test is commented out. If this test is being temporarily disabled, it would be better to use it.skip() to make the intent clear and prevent it from being forgotten. If it's no longer relevant, it should be removed.
wu-hui
force-pushed
the
align-expressions-backend
branch
8 times, most recently
from
ad7cf87 to
b6178c0
Compare
wu-hui
force-pushed
the
align-expressions-backend
branch
from
b6178c0 to
3816c3f
Compare
2 participants
No description provided.