Update mongodbdriver.py #56
Conversation
There was a problem hiding this comment.
Hi @vsarathy1, so far it looks good to me! My comments are mostly nitpicks, and there is some dead code that we should address.
Can we remove the loadConfig method? Right now it just instantly returns and there is a bunch of dead code afterwards.
| for tableName in constants.MONGO_ALL_TABLES: | ||
| self.collections[tableName] = self.database.get_collection(constants.MONGO_COLLECTIONS_DICT[tableName]) | ||
| for tableName in constants.ALL_TABLES: | ||
| self.__dict__[tableName.lower()] = self.database.get_collection(constants.COLLECTIONS_DICT[tableName]) |
There was a problem hiding this comment.
Does this interfere with what looks like something very similar being done on L134?
Is the line here necessary?
Update: oh wait, I've just seen that loadConfig in this class does nothing
| ol_total = 0 | ||
| for oLines in orderLines: | ||
| oLine = oLines["o_orderline"] | ||
| for i in range(len(oLine)): | ||
| o_total = oLine[i]["ol_amount"] | ||
| ol_total += o_total |
There was a problem hiding this comment.
nitpick: this can be made maybe a bit clearer by writing like so:
ol_total = sum(ol["ol_amount"] for o in orderLines for ol in o["o_orderline"])
| o = {"o_id": d_next_o_id, "o_entry_d": o_entry_d, "o_carrier_id": o_carrier_id, "o_ol_cnt": ol_cnt, "o_all_local": all_local} | ||
| o["o_d_id"] = d_id | ||
| o["o_w_id"] = w_id | ||
| o["o_c_id"] = c_id |
There was a problem hiding this comment.
nitpick: we can just write all this in one go:
o = {
"o_id": d_next_o_id,
"o_entry_d": o_entry_d,
"o_carrier_id": o_carrier_id,
"o_ol_cnt": ol_cnt,
"o_all_local": all_local,
"o_d_id": d_id,
"o_w_id": w_id,
"o_c_id": c_id
}
| allStocks = self.stock.find({"s_i_id": {"$in": i_ids}, "s_w_id": w_id}, {"s_i_id": 1, "s_quantity": 1, "s_data": 1, "s_ytd": 1, "s_order_cnt": 1, "s_remote_cnt": 1, s_dist_col: 1}) | ||
| allStocksCount = self.stock.count_documents({"s_i_id": {"$in": i_ids}, "s_w_id": w_id}) | ||
| assert allStocksCount == ol_cnt | ||
| stockInfos = { } | ||
| for si in allStocks: | ||
| stockInfos["S_I_ID"] = si # HACK | ||
| stockInfos["s_i_id"] = si # HACK |
There was a problem hiding this comment.
This is all dead code because of the if all_local and False - do we need this at all or can it be removed?
There was a problem hiding this comment.
This has always been there, let's leave it for now.
| o["o_d_id"] = d_id | ||
| o["o_w_id"] = w_id | ||
| o["o_id"] = d_next_o_id | ||
| o["o_orderline"] = ol |
There was a problem hiding this comment.
nitpick: can be done in one go like
o.update({"o_d_id": d_id, "o_w_id": w_id, "o_id": d_next_o_id, "o_orderline": ol})
| for oLines in orderLines: | ||
| oLine = oLines["o_orderline"] | ||
| for i in range(len(oLine)): | ||
| olid = oLine[i]["ol_i_id"] | ||
| ol_ids.add(olid) |
There was a problem hiding this comment.
nitpick: can be written more simply like so:
ol_ids = set(ol["ol_i_id"] for o in orderLines for ol in o["o_orderline"])
| if bool(int(os.environ.get("IGNORE_SKIP_INDEX_HINTS", 0))): | ||
| pattern = re.compile(r"\/\*\+\sskip-index\s\*\/") | ||
| ch2_queries = { | ||
| k: re.sub(pattern, "", v) for k, v in ch2_queries.items() | ||
| } |
There was a problem hiding this comment.
I suspect this can be removed since Atlas SQL likely doesn't use the same skip-index CBO hints that Couchbase does
| self.database.aggregate(pipeline) | ||
| e = int(time.time_ns()) / 1000000 | ||
| end = time.time() | ||
| dur = str(e-s)+"ms" |
There was a problem hiding this comment.
nitpick: might be simpler to convert to ms only once, here, instead of converting s and e to ms straightaway.
I.e.
s = time.time_ns()
...
e = time.time_ns()
...
dur = str(e-s // 1_000_000) + "ms"
No description provided.