Merge putDtype and putType; remove duplicated code #8610

Noremos · 2025-06-18T14:16:16Z

This is also a preparation for the JSON Type implementation

asfernandes

If you not choose better names for methods you created to not duplicate code, I prefer to have the code duplicated.
I understand nothing what the refactored methods do by their names.

asfernandes · 2025-06-19T00:35:13Z

src/dsql/DsqlCompilerScratch.cpp

+	putType(*type, useSubType, type->collate.object.hasData());
+}
+
+void DsqlCompilerScratch::putType(const TypeClause& type, bool useSubType, bool useExplicitCollate)


It's weird that the two methods with the same name one expects the type as a pointer and the other as reference.

This is a design issue. A pointer means that the value can be null, and the method should check for that. But these methods explicitly treat values as non_null. It would have been better to change the original putType and putDType to use references, but I avoided this to avoid changing too many files.

I'd make the reference-based putType() private to avoid confusion.

And maybe rename putDtype() to putFieldType() or make it a putType() overload with the dsql_fld* argument. Less than a dozen lines changed, I guess.

Noremos · 2025-06-19T06:57:40Z

If you not choose better names for methods you created to not duplicate code, I prefer to have the code duplicated. I understand nothing what the refactored methods do by their names.

I tried to follow the style of the original method names. putType and putDtype are not the most transparent names either. So it would be nice if you could suggest alternative names for my methods

dyemanov · 2025-06-19T07:27:05Z

src/dsql/DsqlCompilerScratch.cpp

+	putType(*type, useSubType, type->collate.object.hasData());
+}
+
+void DsqlCompilerScratch::putType(const TypeClause& type, bool useSubType, bool useExplicitCollate)


I'd make the reference-based putType() private to avoid confusion.

And maybe rename putDtype() to putFieldType() or make it a putType() overload with the dsql_fld* argument. Less than a dozen lines changed, I guess.

dyemanov · 2025-06-19T07:39:48Z

src/dsql/DsqlCompilerScratch.cpp

-void DsqlCompilerScratch::putType(const TypeClause* type, bool useSubType)
+
+template<bool THasTableName>
+void DsqlCompilerScratch::putTypeNameBlr(const TypeClause& type, const bool useExplicitCollate)


All put* methods are already about BLR, so it may be named just putTypeName().

I removed the blr suffix as you suggested.

dyemanov · 2025-06-19T07:42:13Z

src/dsql/DsqlCompilerScratch.cpp

+	putDTypeBlr(type, useSubType);
+}
+
+void DsqlCompilerScratch::putDTypeBlr(const TypeClause& type, const bool useSubType)


AFAIU, this method is used just once. Does it exist just to reduce the number of lines inside putType()? Anyway, it should be private, IMHO.

And if putDtype() above is renamed, then I'd suggest to remove the Blr suffix here (see the reason in the comment below).

I renamed the putDtype to putType as you suggested

Does it exist just to reduce the number of lines inside putType()

Initially, I took out only this method from putType and putDType, but then I thought that this was a half-measure and it was necessary to do a full-fledged refactoring.
For now I've left this function separate, as I hope in the future it will make it easier to refactor and use a single function to generate TypeClause and dsc blr

dyemanov · 2025-06-19T07:44:53Z

src/dsql/DsqlCompilerScratch.cpp

-		ERRD_bugcheck(buffer);
-	}
-#endif
+	static constexpr BlrNameSet BLR_COLUMN_SET{


Here and below - please add the space before the curly brace or move it into a separate line.

dyemanov · 2025-06-19T07:48:18Z

src/dsql/DsqlCompilerScratch.cpp

+
+	constexpr BlrNameSet blrSet = []()
+	{
+		if constexpr(THasTableName)


A space after constexpr, please

dyemanov · 2025-06-19T07:49:54Z

src/dsql/DsqlCompilerScratch.cpp

+		if (useExplicitCollate)
+		{
+			appendUShort(type.textType);
+		}


Here and above: one-liner statement bodies without braces, please.

dyemanov · 2025-06-19T07:54:51Z

src/dsql/DsqlCompilerScratch.cpp

+	if constexpr (THasTableName)
+		differentSchema = type.typeOfTable.schema != ddlSchema;
+	else
+		differentSchema = type.typeOfName.schema != ddlSchema;


Wouldn't something like this possible and maybe better?

const auto typeSchema = constexpr (THasTableName) ? type.typeOfTable.schema : type.typeOfName.schema; if (typeSchema != ddlSchema)

if constexpr is a single construct in C++ hat discards one of the execution branches at compile time. I doubt it can be replaced.

unfortunately, there is no constexpr ternary operator. This is why I used the lambda function for the blrSet

…rivate

Artyom Abakumov added 4 commits June 16, 2025 12:07

Merge putDtype and putType, remove duplicate code

0ef4980

Remove extra line

a8a6739

Simplify typeOfTable and typeOfName choice in blr generation

c6f7d3f

Add tests for DsqlCompilerScratch::putDType

e1da005

Noremos mentioned this pull request Jun 18, 2025

Copy-pasted code in engine #8612

Open

Noremos changed the title ~~Merge putDtype and putType, remove duplicate code~~ Merge putDtype and putType; remove duplicated code Jun 18, 2025

asfernandes requested changes Jun 19, 2025

View reviewed changes

dyemanov reviewed Jun 19, 2025

View reviewed changes

Artyom Abakumov added 4 commits June 19, 2025 13:08

Rename putDtype to putType with dsql_fld* overload

f360f39

Use better names for TypeClause gen blr functions and make putField p…

18b1ce7

…rivate

Resolve code style issues

04a9ae7

Fix unit tests for putType with dsql_fld

8cbc03f

Uh oh!

Merge putDtype and putType; remove duplicated code #8610

Are you sure you want to change the base?

Merge putDtype and putType; remove duplicated code #8610

Uh oh!

Conversation

Noremos commented Jun 18, 2025

Uh oh!

asfernandes 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!

Noremos commented Jun 19, 2025

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!

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!

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!