Prepare C-language infrastructure for ISO:SQL #74

mkgrgis · 2023-05-02T14:31:38Z

No SQL behaviour changes.

Preparing to implement SQL behaviour from table discussed in #66 (comment)

This PR include:

improve naming;
add function for human readable error message about SQLite data value affinity;
change return type for sqlite_convert_to_pg from Datum to NullableDatum for future proper uuid processing and other special cases such as void text affinity in data for int column;
add TODO comments about PostgreSQL database encoding in proper places;
reduce double calculating for two functions: sqlite3_column_bytes and sqlite3_column_bytes for the same data value;
add draft for human readable error message about SQLite data value affinity;
etc.

No SQL behaviour changes. This PR include: * improve naming * add function for human readable error message about SQLite data value affinity * change return type for `sqlite_convert_to_pg` from `Datum` to `NullableDatum` * add TODO comments about PostgreSQL database encoding in proper places * add draft for human readable error message about SQLite data value affinity * etc

nxhai98

I have some comments for your PR. Could you check it?

nxhai98 · 2023-05-10T07:02:20Z

deparse.c

@@ -2092,6 +2092,7 @@ sqlite_deparse_column_option(int varno, int varattno, PlannerInfo *root, char *o
 void
 sqlite_deparse_string_literal(StringInfo buf, const char *val)
 {
+    // TODO: text input for SQLite is always UTF-8, we need to respect PostgreSQL database encoding


Could you check the indent, please indent by Tabs, not Spaces.
And the comment should be in style (Same with Postgres):

/* some comment */

or

/* * some comment */

Fixed in https://github.com/mkgrgis/sqlite_fdw/tree/convert_data_types, also added mkgrgis@7262db8 about C code style.

I checked the change, but it not same as my expectation. Could you refer this patch:
fix_comment_style.patch

I think now all my comments are /t aligned and 2nd+ lines also begins from some /t and one whitespace. Please show a problems not fixed by me.

nxhai98 · 2023-05-10T07:06:42Z

sqlite_fdw.c

-			row[attnum] = sqlite_convert_to_pg(pgtype, pgtypmod,
-											   stmt, attid, festate->attinmeta, attnum);
+		    /* Here will be processing of column options about special convert behaviour 
+		    options = GetForeignColumnOptions(rel, attnum_base);


Is this code unnecessary? If so, please remove it.

This code will be uncommented during implementation of data transformation table. @t-kataym have wrote this implementation need to change SQL behaviour during column and other levelled options. It's important to leave this necessary draft for other contributors. My next PR will be about PostgreSQL encoding, PR after next about human readable error message (uncommenting) and than first PR with data transformation table implementation and introducing a new option or options.

Thank for your explanation,

However, we cannot accept the PR containing such comment code.
Leaving commented-out code in source code is bad practice, as it takes up space, causes confusion, and leads to maintenance issues if it's not removed.

Could you remove it or change it to a TODO note?

Yes, I have reduced this draft to one TODO comment line. It's really "leaving commented-out code in source code is bad practice".

Could you check the other place also?

Yes, please comment on lines after my new commit with missing code style. Now I have no idea where is problems.

nxhai98 · 2023-05-10T07:09:41Z

sqlite_query.c

-		elog(ERROR, "invalid input syntax for type =%d, column type =%d", sqlite_type, col_type);
+	if (affinity_for_pg_column != sqlite_value_affinity && sqlite_value_affinity == SQLITE3_TEXT)
+	{
+	    /* Here will be human readable error message


These codes seem be using for creating a human readable error message, should it be anable?

My next PR will be about PostgreSQL encoding, PR after next about human readable error message (uncommenting). So, this code will be uncommented in a small PR after next small PR if this PR will be accepted.

I don't want to break any tests with the message in this PR, but will uncomment in near future.

nxhai98 · 2023-05-10T07:11:27Z

sqlite_query.c

-				SET_VARSIZE(value_datum, blobsize + VARHDRSZ);
-				return PointerGetDatum(value_datum);
+				// int			value_byte_size_blob_or_utf = sqlite3_column_bytes(stmt, stmt_colid); // Calculated always for detectind void values
+				value_datum = (Datum) palloc0(value_byte_size_blob_or_utf8 + VARHDRSZ);


value_byte_size_blob_or_utf8 is used only in this block, could you explain why we need define it outside?
I do not understand your comment as well: // Compute always, void text and void BLOB will be special cases

In our data transformation table empty value with text affinity is special case. For example for PostgreSQL uuid we should always threat void text value as NULL. For PostgreSQL int we can threat this value as NULL if a special boolean option is true. So for all PostgreSQL data types if (value_byte_size_blob_or_utf8) will be necessary.

Thank for your explanation.
I understood.

nxhai98 · 2023-05-10T07:12:34Z

sqlite_query.c

@@ -111,15 +131,24 @@ sqlite_convert_to_pg(Oid pgtyp, int pgtypmod, sqlite3_stmt * stmt, int stmt_coli
 				valstr = DatumGetCString(DirectFunctionCall1(float8out, Float8GetDatum((float8) value)));
 				break;
 			}
+		/* some popular datatypes for default algorythm branch


Please using comment style:

/* * some comment */

Fixed, please verify.

Fixed again, @nxhai98 !

See pgspider#74 (review)

mkgrgis · 2023-05-11T11:48:46Z

Thanks, @nxhai98 -san! I have fixed some problems in mkgrgis@74bcfea

nxhai98

I add some more comments. Could you please help me check it again?

nxhai98 · 2023-05-16T02:44:42Z

sqlite_fdw.c

@@ -1572,14 +1573,33 @@ make_tuple_from_result_row(sqlite3_stmt * stmt,
 		int			attnum = lfirst_int(lc) - 1;
 		Oid			pgtype = TupleDescAttr(tupleDescriptor, attnum)->atttypid;
 		int32		pgtypmod = TupleDescAttr(tupleDescriptor, attnum)->atttypmod;
+		//List	   *options = NULL;


Please remove it.

nxhai98 · 2023-05-16T02:46:02Z

sqlite_fdw.c

-			is_null[attnum] = false;
-			row[attnum] = sqlite_convert_to_pg(pgtype, pgtypmod,
-											   stmt, attid, festate->attinmeta, attnum);
+		    /* TODO: Processing of column options about special convert behaviour 


This indent is not correct. Please using \t instead of 3 spaces

nxhai98 · 2023-05-16T02:47:46Z

sqlite_query.c

-		elog(ERROR, "invalid input syntax for type =%d, column type =%d", sqlite_type, col_type);
+	if (affinity_for_pg_column != sqlite_value_affinity && sqlite_value_affinity == SQLITE3_TEXT)
+	{
+		/* TODO: human readable error message based on this code


I think the error message format or error message example is enough, not need to add the code here.

Really. I have reduced the comment.

nxhai98 · 2023-05-16T02:48:39Z

sqlite_query.c

 		default:
-			valstr = (char *) sqlite3_column_text(stmt, stmt_colid);
+			{	/*


Please enter the comment to the new line.

nxhai98 · 2023-05-16T02:50:00Z

sqlite_fdw.c

+			 * Flags about special convert behaviour from options on database, table or column level
+			 */
+
+            sqlite_coverted = sqlite_convert_to_pg(pgtype, pgtypmod,


Please using \t instead of spaces.

Here and near fixed

nxhai98 · 2023-05-16T02:50:41Z

sqlite_query.c

@@ -111,15 +131,24 @@ sqlite_convert_to_pg(Oid pgtyp, int pgtypmod, sqlite3_stmt * stmt, int stmt_coli
 				valstr = DatumGetCString(DirectFunctionCall1(float8out, Float8GetDatum((float8) value)));
 				break;
 			}
+		/* some popular datatypes for default algorythm branch


mkgrgis · 2023-05-16T03:57:43Z

I add some more comments. Could you please help me check it again?

Yes. Thanks, @nxhai98 !

nxhai98

Thank for your fixing. I confirmed all the change.

nxhai98 · 2023-05-16T04:14:42Z

sqlite_query.c

-		elog(ERROR, "invalid input syntax for type =%d, column type =%d", sqlite_type, col_type);
+	if (affinity_for_pg_column != sqlite_value_affinity && sqlite_value_affinity == SQLITE3_TEXT)
+	{
+		/* TODO: human readable error message based on this code


t-kataym · 2023-05-16T07:08:44Z

@mkgrgis
@nxhai98's confirmation has completed. We can accept the PR. Can I merge it?

mkgrgis · 2023-05-16T09:26:51Z

@t-kataym , yes, you can squash and merge my PR. Reviews by @nxhai98 was very good and helpful for me! My next little PR will be about database encoding, also without breaking of any tests.

t-kataym · 2023-05-17T05:13:43Z

@mkgrgis , Thank you for your reply and development. I will merge it.

mkgrgis mentioned this pull request May 2, 2023

Many problems with formal text affinity values in foreign tables #66

Closed

nxhai98 reviewed May 10, 2023

View reviewed changes

mkgrgis added 2 commits May 11, 2023 14:45

Merge branch 'pgspider:master' into convert_data_types

9b3c99b

Fix after 1st review

74bcfea

See pgspider#74 (review)

Fix by review and linter notes

90d9064

nxhai98 reviewed May 16, 2023

View reviewed changes

Changes by 2nd PR rewiew

d8e0c8e

nxhai98 approved these changes May 16, 2023

View reviewed changes

t-kataym merged commit 8ad5278 into pgspider:master May 17, 2023

mkgrgis deleted the convert_data_types branch May 17, 2023 05:59

Prepare C-language infrastructure for ISO:SQL #74

Prepare C-language infrastructure for ISO:SQL #74

Conversation

mkgrgis commented May 2, 2023

nxhai98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkgrgis May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkgrgis May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkgrgis commented May 11, 2023

nxhai98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkgrgis commented May 16, 2023

nxhai98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kataym commented May 16, 2023

mkgrgis commented May 16, 2023

t-kataym commented May 17, 2023

mkgrgis May 11, 2023 •

edited

Loading

mkgrgis May 15, 2023 •

edited

Loading