Page MenuHomeSealhub
Differential D213 Diff 717 lib/datastore/query.js

Changeset View

Standalone View

View Options

lib/datastore/query.js

"use strict"; "use strict";
const Promise = require("bluebird"); const Promise = require("bluebird");
const Graph = require("./graph.js");
const object_hash = require("object-hash");
const hash_item = value => const hash_item = value =>
require("object-hash")(value, { object_hash(value, {
algorithm: "md5", algorithm: "md5",
excludeKeys: key => key === "as", excludeKeys: key => key === "as",
}); });
class Query { class Query {
constructor() { constructor() {
this.stages = []; this.stages = [];
} }
lookup(body, unwind = true) { lookup(body, unwind = true) {
body.as = hash_item(body); body.as = hash_item(body);
this.stages.push({ $lookup: body, unwinds: unwind }); this.stages.push({ $lookup: body, unwinds: unwind });
return body.as; return body.as;
} }
match(body) { match(body) {
this.stages.push({ $match: body }); this.stages.push({ $match: body });
return this;
} }
kuba-orlikUnsubmitted
Not Done
Inline Actions

Dlaczego nie zwracamy tutaj już this? Nie można przez to chainować, prawda?

kuba-orlik: Dlaczego nie zwracamy tutaj już `this`? Nie można przez to chainować, prawda?
piotr-ptaszynskiAuthorUnsubmitted
Not Done
Inline Actions

Wydaje mi się, że wprowadzało by to za dużo zamieszania. lookup nie może zwracać this, bo musi zwracać odpowiedniego hasza

piotr-ptaszynski: Wydaje mi się, że wprowadzało by to za dużo zamieszania. `lookup` nie może zwracać `this`, bo…
dump() { dump() {
return this.stages; return this.stages;
} }
toPipeline() { toPipeline() {
return this.stages.reduce( return this.stages.reduce(
(acc, stage) => this._pushToPipeline(acc, stage), (acc, stage) => this._pushToPipeline(acc, stage),
[] []
); );
} }
_pushToPipeline(pipeline, stage) { _pushToPipeline(pipeline, stage) {
if (!stage.$lookup) { if (!stage.$lookup) {
pipeline.push(stage); pipeline.push(stage);
} else { } else {
pipeline.push({ $lookup: stage.$lookup }); pipeline.push({ $lookup: stage.$lookup });
if (stage.unwinds) { if (stage.unwinds) {
pipeline.push({ $unwind: "$" + stage.$lookup.as }); pipeline.push({ $unwind: "$" + stage.$lookup.as });
} }
} }
return pipeline; return pipeline;
} }
static fromSingleMatch(body) { static fromSingleMatch(body) {
const query = new Query(); const query = new Query();
return query.match(body); query.match(body);
return query;
} }
static fromCustomPipeline(stages) { static fromCustomPipeline(stages) {
const query = new Query(); const query = new Query();
query.stages = stages; query.stages = stages;
return query; return query;
} }
_addStages(stages) {
kuba-orlikUnsubmitted
Done
Inline Actions

Jeżeli dobrze rozumiem tę funkcję, to imho nazwa _isUnwindStage pasowałaby lepiej ;)

I mam pytanie - dlaczego poniżej sprawdzamy dokładnie wartość .$unwind? Nie wystarczyłoby sprawdzić, czy to pole istnieje?

kuba-orlik: Jeżeli dobrze rozumiem tę funkcję, to imho nazwa `_isUnwindStage` pasowałaby lepiej ;) I mam…
for (let stage of stages) {
if (stage.$match) {
this._match(stage);
} else if (stage.$lookup) {
this._lookup(stage);
} else {
throw new Error("Unsupported query: " + Object.keys(stage));
}
}
}
} }
Query.DenyAll = class extends Query { Query.DenyAll = class extends Query {
constructor() { constructor() {
super(); super();
super.match({ _id: { $exists: false } }); super.match({ _id: { $exists: false } });
} }
lookup() { lookup() {
Show All 22 Lines constructor(...queries) {
super(); super();
this.lookups = {}; this.lookups = {};
this.matches = []; this.matches = [];
for (let query of queries) { for (let query of queries) {
this.addQuery(query); this.addQuery(query);
} }
} }
addQuery(query) { addQuery(query) {
let stages = query.dump(); const stages = query.dump();
for (let stage of stages) { this._addStages(stages);
if (stage.$lookup) {
this._lookup(stage);
} else if (stage.$match) {
this._match(stage);
} else {
throw new Error("Unsupported query: " + Object.keys(stage));
}
}
} }
_lookup(stage) { _lookup(stage) {
const id = stage.$lookup.as; const id = stage.$lookup.as;
this.lookups[id] = stage; this.lookups[id] = stage;
} }
_match(stage) { _match(stage) {
this.matches.push(stage); this.matches.push(stage);
} }
dump() { dump() {
return Object.values(this.lookups).concat({ return Object.values(this.lookups).concat({
$match: { $or: this.matches.map(stage => stage.$match) }, $match: { $or: this.matches.map(stage => stage.$match) },
}); });
} }
toPipeline() { toPipeline() {
return Object.values(this.lookups) return Object.values(this.lookups)
.reduce((acc, stage) => this._pushToPipeline(acc, stage), []) .reduce((acc, stage) => this._pushToPipeline(acc, stage), [])
.concat({ .concat({
$match: { $or: this.matches.map(stage => stage.$match) }, $match: { $or: this.matches.map(stage => stage.$match) },
}); });
} }
}; };
Query.And = class extends Query {
kuba-orlikUnsubmitted
Done
Inline Actions

Plik query.js stał się bardzo duży. Warto AND przenieść do osobnego pliku. Jeżeli pojawią się cykliczne zależności, to pomoże trick:

module.exports = Query;

Query.And = require("./and.js")

(module.exports przed requirem)

kuba-orlik: Plik `query.js` stał się bardzo duży. Warto `AND` przenieść do osobnego pliku. Jeżeli pojawią…
kuba-orlikUnsubmitted
Done
Inline Actions

Strategii dostępu And brakuje jeszcze trochę elegancji. Podejrzewam że jest to spowodowane tym, że ta klasa ma dużo metod odpowiedzialnych za stwierdzanie, czy dany "stage" znajduje się w grafie itp. Myślę, że jak się je przeniesie do osobnej klasy odpowiedzialnej za pośrednią reprezentację "stage"'ów to niniejsza klasa zyska na elegancji

kuba-orlik: Strategii dostępu `And` brakuje jeszcze trochę elegancji. Podejrzewam że jest to spowodowane…
constructor(...queries) {
super();
this.graph = new Graph();
this.aggregation_steps = {};
for (let query of queries) {
this.addQuery(query);
}
}
addQuery(query) {
const stages = query.dump();
if (query instanceof Query.Or) {
this._addOrPipeline(stages);
return;
}
kuba-orlikUnsubmitted
Done
Inline Actions

(should) dlaczego traktujemy Or wyjątkowo? Myśle, że warto unikać takich przypadków brzegowych ;)

kuba-orlik: (should) dlaczego traktujemy `Or` wyjątkowo? Myśle, że warto unikać takich przypadków…
this._addStages(stages);
}
_addOrPipeline(pipeline) {
const id = hash_item(pipeline);
const priority = this._evaluateOrPipelinePriority(pipeline);
this._addToAggregationSteps(id, pipeline, priority);
kuba-orlikUnsubmitted
Done
Inline Actions

(question) Z tego co widzę, nie rozbijamy tutaj query na osobne $match-e, tak jak to ma w przypadku metodu _match. Czy to jest zamierzone?

kuba-orlik: (question) Z tego co widzę, nie rozbijamy tutaj query na osobne `$match`-e, tak jak to ma w…
}
_evaluateOrPipelinePriority(pipeline) {
return pipeline[0].$match ? 2 : 16;
}
_match(stage) {
for (let key of Object.keys(stage.$match)) {
const step = { $match: { [key]: stage.$match[key] } };
const id = hash_item(step.$match);
if (this.graph.node_ids.includes(id)) {
return;
}
this._addToAggregationSteps(id, step, Graph.MAX_PRIORITY);
const paths = getAllKeys(step.$match);
const dependencies = paths
.map(path => path.split("."))
.reduce((acc, fields) => {
return acc.concat(
fields.filter(field => this._isInGraph(field))
);
}, []);
this._addDependenciesOnlyFromFinalNodes(id, dependencies);
}
}
_isInGraph(key) {
return (
!key.startsWith("$") &&
key.length === 32 &&
this.graph.node_ids.includes(key)
);
}
_addDependenciesOnlyFromFinalNodes(id, candidates) {
candidates
.filter(d1 => this._isNotDependencyForAnyInGroup(d1, candidates))
.forEach(dependency => this.graph.addEdge(dependency, id));
}
_isNotDependencyForAnyInGroup(id, nodeGroup) {
return !nodeGroup.some(
node => id !== node && this.graph.pathExists(id, node)
);
}
_lookup(stage) {
const id = stage.$lookup.as;
kuba-orlikUnsubmitted
Done
Inline Actions

(should) Wciąż myli mi się, czy mamy do czynienia z stage w znaczeniu Mongowym, czy stage w znaczeniu "nasza wewnętrzna reprezentacja kroku agregacji". Myślę, że można to naprawić nazwami zmiennych - stage zostawiłbym do mongowych kroków agregacji, a dla naszej wewnętrznej stworzył nową nazwę - albo nawet klasę

kuba-orlik: (should) Wciąż myli mi się, czy mamy do czynienia z `stage` w znaczeniu Mongowym, czy `stage` w…
if (this.graph.node_ids.includes(id)) {
return;
}
this._addToAggregationSteps(id, stage, 8);
const candidatesForDependencies = stage.$lookup.localField.split(".");
for (let candidate of candidatesForDependencies) {
if (this._isInGraph(candidate)) {
this.graph.addEdge(candidate, id);
}
}
}
_addToAggregationSteps(id, step, priority) {
this.graph.addNode(id, priority);
this.aggregation_steps[id] = step;
}
toPipeline() {
const sortedStepIds = this.graph.bestFirstSearch();
return sortedStepIds.reduce((pipeline, id) => {
if (Array.isArray(this.aggregation_steps[id])) {
for (let step of this.aggregation_steps[id]) {
this._pushToPipeline(pipeline, step);
}
return pipeline;
}
return this._pushToPipeline(pipeline, this.aggregation_steps[id]);
}, []);
}
};
function getAllKeys(obj) {
return Object.keys(obj).reduce((acc, key) => {
if (obj[key] instanceof Object) {
acc.push(...getAllKeys(obj[key]));
}
if (!Array.isArray(obj)) {
acc.push(key);
}
return acc;
}, []);
}
kuba-orlikUnsubmitted
Not Done
Inline Actions

Przy testach zdałem sobie sprawę, że Query.Not nie działa poprawnie:

> a = new Query();
Query { steps: [] }

> a.match({a: "A"})
undefined

> a.dump()
[ QueryStep { body: { a: 'A' } } ]

> const b = new Query()
undefined

> b.match({b: "B"})
undefined

> b.dump()
[ QueryStep { body: { b: 'B' } } ]

> new Query.Not(new Query.And(a, b)).dump()
[{"body":{"a":{"$not":"A"}}},{"body":{"b":{"$not":"B"}}}]

W skrócie: ~(a ^ b) zwraca ~a ^ ~b, kiedy powinno ~a OR ~b

Nie wiem, czy ta uwaga powinna tyczyć się tego konkretnie diffa, czy lepiej założyć na to osobnego taska - ale na pewno trzeba to poprawić. Zrób wedle uznania - jeżeli uważasz, że lepiej w osobnym tasku, to proszę od razu takowego załóż :)

kuba-orlik: Przy testach zdałem sobie sprawę, że `Query.Not` nie działa poprawnie: ``` > a = new Query()…
module.exports = Query; module.exports = Query;