Comments (11)
@MironAtHome Also, you are welcome to submit a PR for a change to make things more consistent. So long as it builds without issues and is consistent, then it should be accepted.
from age.
Refactored functions available in the fork, under respective branches:
branch port/win32msvc/PG14/1.5.1 is_valid_label_name
branch port/win32msvc/PG16/1.5.1 is_valid_label_name
branch port/win32msvc/PG14/1.5.1 is_valid_label
branch port/win32msvc/PG16/1.5.1 is_valid_label
from age.
@MironAtHome The function is_valid_label
is an internal function that is part of name validation. For example, it is used by is_valid_label_name
. Its function is not to validate whether a label exists, rather it is to validate the proper construction of the name itself. This is why it doesn't need to know the graph name.
/*
* Returns whether the label name is valid.
*
* Note: label_type parameter is not used in this implementation.
* It should be used if validation algorithm for edge and vertex
* differs in future.
*
* @param label_name name of the label
* @param label_type label type defined in label_commands.h
* @return int
*/
int is_valid_label(char *label_name, char label_type)
PG_FUNCTION_INFO_V1(age_is_valid_label_name);
Datum age_is_valid_label_name(PG_FUNCTION_ARGS)
{
agtype *agt_arg = NULL;
agtype_value *agtv_value = NULL;
char *label_name = NULL;
bool is_valid = false;
...
is_valid = is_valid_label(label_name, 0);
pfree(label_name);
if (is_valid)
{
PG_RETURN_BOOL(true);
}
PG_RETURN_BOOL(false);
}
from age.
@MironAtHome OidIsValid
is a macro to check if an Oid is valid, and only that -
#define OidIsValid(objectId) ((bool) ((objectId) != InvalidOid))
InvalidOid
is always a zero value -
#define InvalidOid ((Oid) 0)
While label_id_is_valid
is validating that a label id is within a range that is within 32 bits, 16 to be precise -
#define PG_UINT16_MAX (0xFFFF)
#define LABEL_ID_MIN 1
#define LABEL_ID_MAX PG_UINT16_MAX
#define INVALID_LABEL_ID 0
#define label_id_is_valid(id) (id >= LABEL_ID_MIN && id <= LABEL_ID_MAX)
from age.
I have 2 separate questions regarding above
#1 is_valid_label
and
#2 label_id_is_valid ( with regards to PG_UINT16_MAX )
I tried best I could to clarify both questions from the start, but it looks like things got a bit clobbered, so, let me try one more attempt. This is only to better clarify and see what your / team thinking is regarding same questions asked above.
So, let's start from question number 1 :)
Yes, there is a function is_valid_label and, just as you have pointed out, it currently does cater to label name.
However, there is one more function, and taking into account function mentioned in your response, age_is_valid_label_name, function, there are 2 functions total catering to label name validation, one internal and one exposed as part of SQL interface.
Now, if you look into file name_validation.c you will find that it currently carries 2 ( non - static ) functions:
Function number 1: is_valid_graph_name
Function number 2: is_valid_label
So, what I am trying to say is that it would be very - very nice to make functions named consistent, so that both were named according to their function, similarly, and so, after the change the two function names will be
Function number 1: is_valid_graph_name
Function number 2: is_valid_label_name
Please note, an static function with name is_valid_label currently does not exist in the unit label_commands.c
So, my suggestion is to create is_valid_label and have it cater to label validity. Whether it makes sense to make it not - static and available, as part of the C interface, it would be something to weight. On my mind it makes sense to have a function like this available throughout, with its declaration in header file.
In addition, for consistency, it would be very - very nice to add to graph_commands.c function
age_is_valid_graph_name, And if you could follow this path of thought, it would be very - very nice to add check for valid name to each function, where either external user or internal call requests some operation on graph or label name, to defend code from some request with some name, that could, potentially, cause trouble.
Please note, I am not trying to refer to something like naming convention. My hope is to instill consistency into interface.
To note, it does make sense to make functions, checking validity of name or object return boolean. Currently those return integer.
Please do accept all what I said with a grain of salt and my apologies ahead of time for redacting away in each sentence "in my opinion", else, this comment will ran twice as long :)
from age.
Regarding question number 2 [ label_id_is_valid ( with regards to PG_UINT16_MAX ) ]
I guess, it would be very nice, if you could please help to understand, whether Age considers label id to be an Oid with range from 0 to (2^32 - 1) or something else.
It seems to me, based on your answer above "something else" is correct, but than there are two separate checks for the label id makes for a great deal of confusion.
While both checks
Check number 1: OidIsValid
Check number 2: label_id_is_valid
would render label id 0 is failing check, the two carry very different suggestion with regards to what would pass the test.
Check number 1 will pass with success from 1 to (2*32 - 1)
Check number 2 will pass from 1 to 65535
On my mind for consistency sakes it would be nice to have one check for correctness of label id.
In an ideal world what I would really like to learn from you in this question, first, before seeing any change needed or not needed, is how you see the data model, what is the true meaning of this label id,
Is it a row id of a record in edge table ( in which case it should probably be generated by a sequence, unless user wants it to be something specific, when, for example, loading from file )?
Is it a type id of a distinct edge? ( in which case this id should probably repeat in each applicable row for the id field )
I am honestly asking this question from the position of "unaware" user, who looks for guidance, in kind, before anything else.
The two different checks and the limit value set to 65535, seem to convey enough confusion, to ask for guidance first.
from age.
@MironAtHome I can clarify more of this tomorrow, but I feel I should mention the following,...
Only functions, like below, are available to SQL or Cypher, all the others are only available for people contributing to the codebase -
PG_FUNCTION_INFO_V1(create_vlabel);
The prefix age_
is used to help eliminate naming conflicts, group our functions together, and allow them to be used in Cypher. C would have all kinds of fits if we tried to redefine an existing library function.
And, yes, it would be nice to have consistency with naming. Unfortunately, with many differently experienced cooks, come many flavors.
from age.
Will do.
from age.
@MironAtHome Given the size of our team, those who actively contribute to the project, the size of the codebase, it can be difficult to enforce naming conventions and where functions are located. And, of course, get anything done at all :) This is especially true when it comes to developer experience. Meaning, knowing where something 1) should be, 2) if it exists elsewhere, or 3) the proper way to build it in PG and AGE. So, I welcome people who want to contribute and bring our codebase more into line when they find these issues.
If you feel a change would be useful, please provide a PR for the master branch, for review, and we can move on from there.
edit: and even experienced devs make mistakes.
from age.
Changed to enhancement as this isn't really a bug.
from age.
Related Issues (20)
- Is data duplicated in parent and label/edge tables? What are the internals? HOT 2
- Age graph existance check function, something like "graph_exists" would be very nice HOT 7
- Could you please help with example, how to correctly use age_graph_stats function HOT 3
- Convert vertex:: or edge:: into text or jsonb types HOT 3
- Asking community for input regarding setup GraphQL API server to integrate with Apache Age Graph Database ecosystem, more questions on same inside HOT 1
- How to allow special characters in labels, backticks only works for property names. HOT 1
- Server crashes when executing SELECT * FROM cypher('ag_graph_3'...) HOT 3
- Problem with the WITH clause: Vertex assigned to variable <x> was deleted. HOT 2
- List comprehension in WHERE clause results in server process segmentation fault HOT 3
- Server crashes when executing SELECT agtype_build_map('null'::agtype, 1); HOT 3
- Server crashes when executing PREPARE property_ps(agtype)...
- MATCH Query Syntax issues HOT 3
- Simple query takes long time to run HOT 12
- In Greenplum, compiling the AGE plugin fails with the error: 'void value not ignored as it ought to be HOT 2
- load_labels_from_file produced duplicated graphId HOT 3
- cypher returned agtype cannot work with postgres IN clause HOT 1
- One more request for exist type function, this time for vertex and edge, previous was for graph. HOT 1
- Failure creating vertex with name close to MAX_LABEL_NAME_LEN 65535 HOT 7
- How to tune age-viewer to load graph faster HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from age.