iframe-proxy

alonre24 · 2020-10-11T15:22:58Z

This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE).
#1376

… This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE).

DvirDukhan · 2020-10-11T15:40:05Z

+#include "proc_procedures.h"
+#include "../util/arr.h"
+#include "rax.h"
+#include "procedure.h"


General formatting comment:
Order the include statement according to line length

DvirDukhan · 2020-10-11T15:41:49Z

+	raxIterator *current_proc; // Current proc
+	SIValue *output; // Array with a maximum of 4 entries: ["name", name, "mode", mode].


General struct formatting:

take the longest line and write the comment 4 spaces from its end. Align all other lines accordingly.

try to sort variables line by length, when possible

DvirDukhan · 2020-10-11T15:42:35Z

+} ProceduresContext;
+
+ProcedureResult Proc_ProceduresInvoke(ProcedureCtx *ctx,
+  const SIValue *args, const char **yield) {


indentation?

DvirDukhan · 2020-10-11T15:50:55Z

 }

+const char *Procedure_GetName(const ProcedureCtx *proc) {
+	assert(proc);


Please include "RG.h" and use ASSERT macro
This is debugging time assert

DvirDukhan · 2020-10-11T15:52:00Z

+// Returns the rax that contains all the procedures
+rax *Proc_Get_All();
+
+// Returns true if the procedure is read-only
+bool Procedure_IsReadOnly(const ProcedureCtx *proc);
+
+//Returns the procedure's name


General comments formatting:
A sentence starts with capital letters and ends with a period.

DvirDukhan · 2020-10-11T15:52:13Z

+}
+
+bool Procedure_IsReadOnly(const ProcedureCtx *proc) {
+	assert(proc);


DvirDukhan · 2020-10-11T15:53:50Z

+        # The following two procedure are a part of the expected results
+        expected_result = ["db.labels", "READ"]
+        self.env.assertContains(expected_result, actual_resultset)
+        expected_result = ["db.idx.fulltext.createNodeIndex", "WRITE"]
+        self.env.assertContains(expected_result, actual_resultset)


can you make it a single array?

I don't think so, because the assertContains utility works only with searching a singleton in a set

DvirDukhan · 2020-10-11T15:56:41Z

+	return PROCEDURE_OK;
+}
+
+SIValue *Proc_ProceduresStep(ProcedureCtx *ctx) {


Add comments explaining what the step does

DvirDukhan · 2020-10-12T10:55:29Z

+	return PROCEDURE_OK;
+}
+
+// Promote the rax iterator to the next procedure and return its name and mode


Suggested change

// Promote the rax iterator to the next procedure and return its name and mode

// Promote the rax iterator to the next procedure and return its name and mode.

DvirDukhan · 2020-10-12T10:55:46Z

+
+// Promote the rax iterator to the next procedure and return its name and mode
+SIValue *Proc_ProceduresStep(ProcedureCtx *ctx) {
+	assert(ctx->privateData);


DvirDukhan · 2020-10-12T10:58:32Z

+const char *Procedure_GetName(const ProcedureCtx *proc) {
+	ASSERT(proc != NULL);
+	return proc->name;
+}
+
+bool Procedure_IsReadOnly(const ProcedureCtx *proc) {
+	ASSERT(proc != NULL);
+	return proc->readOnly;
+}
+


Since the only caller to those functions is proc_procedures
consider moving them there as auxiliary static functions

op_procedure_call.c access procedure's readOnly attribute,
please change op_procedure_call.c to call this function instead.

DvirDukhan · 2020-10-12T10:59:15Z

+        expected_result = [["db.labels", "READ"], ["db.idx.fulltext.createNodeIndex", "WRITE"]]
+        for res in expected_result:
+            self.env.assertContains(res, actual_resultset)


Please make sure all current procedures show up in the result-set.

swilly22

Overall good job, please see comments.

swilly22 · 2020-10-12T18:49:29Z

+        expected_result = [["db.labels", "READ"], ["db.idx.fulltext.createNodeIndex", "WRITE"]]
+        for res in expected_result:
+            self.env.assertContains(res, actual_resultset)


Please make sure all current procedures show up in the result-set.

swilly22 · 2020-10-12T18:50:11Z

 #include "proc_fulltext_drop_index.h"
 #include "proc_fulltext_create_index.h"
-
+#include "proc_procedures.h"


lines should be sorts by length.

swilly22 · 2020-10-12T18:56:03Z

+// Returns the rax that contains all the procedures.
+rax *Procedure_GetProceduresRax();


Please remove, Have the procedures procedure access __procedures directly. (use extern)

swilly22 · 2020-10-12T18:56:28Z

+// Returns true if the procedure is read-only.
+bool Procedure_IsReadOnly(const ProcedureCtx *proc);
+
+//Returns the procedure's name.


Space after /

swilly22 · 2020-10-12T19:09:06Z

+	ProcProceduresPrivateData *pdata = rm_malloc(sizeof(ProcProceduresPrivateData));
+
+	// Initialize an iterator to the rax that contains all procedures
+	rax *rax = Procedure_GetProceduresRax();


rax *procedures = __procedures

swilly22 · 2020-10-12T19:09:15Z

+
+	// Initialize an iterator to the rax that contains all procedures
+	rax *rax = Procedure_GetProceduresRax();
+	raxIterator *it = rm_malloc(sizeof(raxIterator));


swilly22 · 2020-10-12T19:09:46Z

+	// Initialize an iterator to the rax that contains all procedures
+	rax *rax = Procedure_GetProceduresRax();
+	raxIterator *it = rm_malloc(sizeof(raxIterator));
+	raxStart(it, rax);


raxStart(it, &pdata->iter);

swilly22 · 2020-10-12T21:09:09Z

+	// Returns the current procedure's name and mode.
+	ProcGenerator gen = it->data;
+	ProcedureCtx *curr_proc_ctx = gen();
+	pdata->output[1] = SI_DuplicateStringVal(Procedure_GetName(curr_proc_ctx));


No need to duplicate procedure name, as the name is a static string.

swilly22 · 2020-10-12T21:09:51Z

+	if(ctx->privateData) {
+		ProcProceduresPrivateData *pdata = ctx->privateData;
+		array_free(pdata->output);
+		rm_free(pdata->procedure_rax_it);


procedure_rax_it should be heap alocated.

…proc_proceudres_private_data. More PR code fixes

swilly22 · 2020-10-13T09:18:58Z


 /* Returns true if procedure is read-only. */
-bool Proc_ReadOnly(const char *proc_name);
+bool Procedure_ReadOnly(const char *proc_name);


Now that we have Procedure_IsReadOnly do we really need both Procedure_IsReadOnly and Procedure_ReadOnly can we do with just one?

…name, leaving only the getter "Procedure_IsReadOnly" that gets the whole procedure object.

* added init time NodeScanCtx update on label scan (#1358) * added init time NodeScanCtx update on label scan * rephrase comment Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Bfs procedure call (#1306) * Import LAGraph files * Apply autoformatter * Add LAGraph include file * WIP Add BFS procedure * tmp Utility * Revert LAGraph inclusions * Replace LAGraph reference with GraphBLAS references * WIP * WIP * Add BFSTree procedure * Add unit tests * Update documentation * Resolve compiler warnings * Address PR comments * Make singular BFS procedure with variable outputs * Remove start_node yield * Address PR comments * Remove yield-edges BFS input arg * Address PR comments * Switch to LAGraph_bfs_pushpull * Remove unused code paths from LAGraph BFS algorithm * procedure invocation receives yield array * consume BFS result using iterator * test BFS returning no results * clean up, address PR comments Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Separate ExecutionPlan op construction logic into different files (#1352) * Separate ExecutionPlan op construction logic into different files * Address PR comments * Refactor header files * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Support array properties in bulk loader (#1365) * Support array properties in bulk loader * Address PR comments * Add no_multi_key & hash_policy (#1363) * Add no_multi_key & hash_policy * Update ramp.yml At the moment we can't handle user hash policy due to virtual keys Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Add LGTM (#1367) * read only query (#1364) * read only query * added sleep, try to avoid ci failure * added more write scenarios. added replica shutdown * added skip flag to avoid memcheck failure * fixed PR comments * Update module.c * try to skip on any debugger. added more write scenarios * checkSlaveSynced moved to use time.sleep. Another try to skip debuger * added RO_QUERY to commands Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Fix typo (#1368) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Add GRAPH.BULK documentation and link to bulk loader documentation (#1371) * Add GRAPH.BULK documentation and link to bulk loader documentation * Address PR comments * Allow upper case letters in proc names (#1372) * added to_lower for procedure name in Proc_get to support both upper and lower cases * new test for lookup_case_insensitive * Created strutil and moved the implementations of str_tolower and str_toupper to it. Remove the static implementations of these functions and replaced every call with a new call for str_tolower. minor changes in getProc and procRegister due to PR * fix indentations * fix indentations * minor changes * minor changes * indentations fix * indentations fix * indentations fix * indentations fix * indentations fix * PR fixes * PR fixes * PR fixes * PR fixes * PR fixes * PR fixes * PR fixes * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Adding a new procedure that returns all the procedures in the system (#1377) * Adding a new procedure that returns all the procedures in the system. This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE). * PR fixes * Saving raxIterator reference instead of allocating it on the heap in proc_proceudres_private_data. More PR code fixes * Remove the getter "Procedure_ReadOnly" that gets only the procedures name, leaving only the getter "Procedure_IsReadOnly" that gets the whole procedure object. * Always set GraphCtx in QueryCtx while decoding graph keys (#1381) * LGTM alert fixes (#1383) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Avoid double free of heap-allocated values in CASE...WHEN statements (#1386) * Avoid double free of heap-allocated values in CASE...WHEN statements * Update test_function_calls.py Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * With filter scoping (#1361) * Don't migrate WITH filters into Merge and Apply scopes * Don't place filters in previous scope for filtered aliases * Fix memory error * remove vector * consolidate operation locating * execution plan construction refactoring WIP * move arithmetic expression construction from AST to AR_EXP * discover skip, limit and black-listed ops modifiers * PR fixes * Fixes continued * Fix ResultSet updates in UNION queries * Clarify comments, minor cleanup * Fix unit tests * Fix caching logic for SKIP and LIMIT * Address PR comments * tidy up execution plan construction and join op * reset limit on aggregation * define 16 as the default batch size for traversal operations Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Updates to Astyle config file (#1384) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * replace gitter with Discord (#1395) * reaplace gitter with Discord * replace gitter to discord * Allow property accesses on non-identifier entity references (#1389) * Allow property accesses on non-identifier entity references * access entity attribute only via the property function * address PR comments Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Minor suggested update. (#1398) * replace automation service (#1399) * redundent filter construction and placement (#1397) * redundent filter construction and placement * Add validating flow tests Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Parse full argv array on main thread (#1298) * Parse full argv array on main thread * remove read_flags from cmd_query Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: swilly22 <roi@redislabs.com> * query validation should not check if procedure outputs been defined (#1406) * query validation should not check if procedure outputs been defined * address PR comments * Add trusted-by logos (#1412) * Add trusted-by logos * add more logos * Fuzzy test crashes (#1408) * Correctly place filters without referenced variables * Emit compile-time error on constant non-boolean filter * Validate references per clause instead of per scope * Emit error on failed filter placement in optimize_cartesian_product * Fix free of invalid value in OpUnwind * Handle NULL sources in variable-length traversals * Address PR comments * Post-rebase fixes * Address PR comments * introduce AST unsupported op error Co-authored-by: swilly22 <roi@redislabs.com> * Graph version (#1382) * add graph version to graph context object * change graph version from string to uint32 * testing graph version * response with an error when graph version mismatch * release graph context on version mismatch * address PR comments * increase command arity to accomadate graph version argument * address PR comments Co-authored-by: DvirDukhan <dvir@redislabs.com> * Release version 2.2.8. Merged from master. Co-authored-by: DvirDukhan <dvir@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Yahya <5457202+anakaiti@users.noreply.github.com> Co-authored-by: Simon Prickett <simon@crudworks.org>

…edisGraph#1377) * Adding a new procedure that returns all the procedures in the system. This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE). * PR fixes * Saving raxIterator reference instead of allocating it on the heap in proc_proceudres_private_data. More PR code fixes * Remove the getter "Procedure_ReadOnly" that gets only the procedures name, leaving only the getter "Procedure_IsReadOnly" that gets the whole procedure object.

Adding a new procedure that returns all the procedures in the system.…

fd2ab90

… This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE).

alonre24 added the new feature label Oct 11, 2020

alonre24 requested review from DvirDukhan, jeffreylovitz and swilly22 October 11, 2020 15:22

alonre24 self-assigned this Oct 11, 2020

DvirDukhan reviewed Oct 11, 2020

View reviewed changes

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch 2 times, most recently from c214890 to e919a70 Compare October 12, 2020 09:02

DvirDukhan reviewed Oct 12, 2020

View reviewed changes

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch from 9d7e577 to cc00897 Compare October 12, 2020 11:29

PR fixes

d55a69a

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch from cc00897 to d55a69a Compare October 12, 2020 15:48

DvirDukhan previously approved these changes Oct 12, 2020

View reviewed changes

swilly22 suggested changes Oct 12, 2020

View reviewed changes

alonre24 dismissed DvirDukhan’s stale review via 8c7e962 October 13, 2020 09:07

Saving raxIterator reference instead of allocating it on the heap in …

6d58b17

…proc_proceudres_private_data. More PR code fixes

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch from 8c7e962 to 6d58b17 Compare October 13, 2020 09:10

swilly22 suggested changes Oct 13, 2020

View reviewed changes

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch 2 times, most recently from 6438816 to e35666e Compare October 13, 2020 11:34

Remove the getter "Procedure_ReadOnly" that gets only the procedures …

65ecf17

…name, leaving only the getter "Procedure_IsReadOnly" that gets the whole procedure object.

alonre24 force-pushed the Add_a_procedure_that_returns_all_procedures branch from e35666e to 65ecf17 Compare October 13, 2020 12:01

swilly22 approved these changes Oct 13, 2020

View reviewed changes

swilly22 merged commit cd97f6c into master Oct 13, 2020

swilly22 deleted the Add_a_procedure_that_returns_all_procedures branch October 13, 2020 13:17

This was referenced Oct 13, 2020

Allow to retrieve all the procedure that exist in the system #1376

Closed

fix procedure read only lookup #1378

Closed

		raxIterator *current_proc; // Current proc
		SIValue *output; // Array with a maximum of 4 entries: ["name", name, "mode", mode].

	// Promote the rax iterator to the next procedure and return its name and mode
	// Promote the rax iterator to the next procedure and return its name and mode.

		// Returns the rax that contains all the procedures.
		rax *Procedure_GetProceduresRax();

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

alonre24 commented Oct 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swilly22 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants