Some small tweaks to Red. by xinminglai · Pull Request #404 · FCO/Red

xinminglai · 2019-11-02T13:22:47Z

Hi, These 3 patches add method column-values, and some small fixes for Red::Driver::Pg

Thanks!

FCO · 2019-11-02T14:40:29Z

It seems 2 tests are failing with these changes...

FCO · 2019-11-02T14:43:42Z

 }

+multi method translate(Red::AST::In $_, $context?) {
+    if .right.value ~~ Positional {


Could you, please, put this test on the signature and remove the else?

I mean something like:

multi method translate(Red::AST::In $_ where .right.value ~~ Positional, $context?) {

But, instead of testing the right's value type, it would be better to test if it's an Red::AST::Value and if its .type is Positional.

multi method translate(Red::AST::In $_ where .right ~~ Red::AST::Value && .right.type ~~ Positional, $context?) {

FCO · 2019-11-02T14:54:16Z

+        my ($lstr, @lbind) := do given self.translate: .left,  $context { .key, .value }
+
+        if .right.value.elems == 0 {
+            return "$lstr { .op } (SELECT 0 WHERE false)" => @lbind;


Why change to use in (SELECT 0 WHERE false)? why not in ()?

Because In Pg, the in () will raise error, So, we should add placehoder statement to select "nothing".

In that way, maybe we could do an:

multi method translate(Red::AST::In $_ where .right ~~ Red::AST::Value && .right.type ~~ Positional && .right.elems, $context?) {

?

FCO · 2019-11-02T15:04:47Z

    self.Red::Driver::CommonSQL::translate($_, $context, :gambi);
 }

+multi method translate(Red::AST::In $_, $context?) {


Isn't it common? Couldn't it be on CommonSQL?

It's common, But It's strange that it works in SQLite.

FCO · 2019-11-02T15:38:51Z

+            return "$lstr { .op } (SELECT 0 WHERE false)" => @lbind;
+        }
+
+        my $in-placeholder = '(' ~ (self.wildcard xx .right.value.elems).join(',') ~ ')';


If you fix one of my errors (https://github.com/FCO/Red/blob/master/lib/Red/Driver/CommonSQL.pm6#L521) changing "?" to self.wildcard, you could just use the self.translate: .right, $context

FCO · 2019-11-02T15:42:18Z

 multi method default-type-for(Red::Column $                                                 --> Str:D) {"varchar(255)"}

+multi method type-by-name("text" --> "text") {}
+multi method type-by-name("json" --> "json") {}


Could you, please, add a test for json and jsonb?

for SQLite, it re-fetches the row from the model by using LastInsertedRow class, Then the `.row{@ids}` does the right thing all the time. But in postgres, the $data will filled by `INSERT INTO table xxx RETURNING *` after the RETURNING, we'll $data with column names with underscores. So, in the pg case, We convert the $data structure according to the model, make $data look like the same as fetched with LastInsertedRow class.

FCO · 2020-01-17T00:45:23Z

+
+model Category is table<test_category> {
+    has Int $.id is serial;
+    has Int $.parent_id is column{ :references{ Category.id }, :nullable, };


This test is failing here. For Red:api<2> you need to set the :model<> for is referencing or :model-name<> in this case... and receive the model type as parameter:

has Int $.parent_if is column{ :references{ .id }, :nullable };

hermes-fco · 2026-06-22T12:42:53Z

Code Review — Some small tweaks to Red.

Verdict: Comment (0 critical, 5 warnings, 2 suggestions)

Small but useful changes adding column-values method, fixing PG in operator with wildcard delegation, adding type-by-name for text/json/jsonb, and fixing an immutability issue in .^create. 4 files, +64/−3. 7 commits from 2019–2020.

⚠️ Note: This PR is over 5 years old and the author hasn't updated since 2020-01. FCO requested changes in 2019 that were never addressed. This review evaluates the code as-is for archival/merge consideration.

✅ Looks Good

lib/Red/Driver/CommonSQL.pm6:526 — Replacing hardcoded '?' with self.wildcard is a clean abstraction. Allows PG driver to use $1/$2 placeholders while SQLite keeps ?.
lib/MetamodelX/Red/Model.pm6:454-458 — The %data-copy fix correctly handles the case where row data uses column names vs attribute names, and avoids Raku immutability errors on row results.
lib/Red/Driver/Pg.pm6:118-120 — type-by-name additions for text, json, jsonb use idiomatic Raku value-type return annotations ((--> "text")). Minimal and correct.

⚠️ Warnings

t/20-in-pg.t:10-13 — Test file is PG-only and silently skips when RED_PG_TEST_DB env var is unset. This means CI won't run these tests unless the env var is configured. Consider adding a done-testing note or a skip message that's visible in CI logs.
t/20-in-pg.t:34-43 — ~7 lines of commented-out exploratory code left in the test file. Should be removed before merging — it adds noise and confusion.
No test for column-values — The new public method in MetamodelX::Red::Model has zero test coverage. A simple test verifying the returned hash structure would improve confidence.
No test cleanup — The test creates test_category table and records but never drops them. While consistent with other Red test files, this leaves artifacts in the test database.
FCO flagged test failures in 2019 — The review from Nov 2019 noted "2 tests are failing with these changes." This was never resolved by the author and should be verified before any merge.

💡 Suggestions

t/20-in-pg.t:10-13 — Use skip-rest inside a without $*RED-PG-TEST-DB block for cleaner Raku idiom:

without $*RED-PG-TEST-DB {
    skip-rest "No RED_PG_TEST_DB env var set";
    exit;
}

lib/MetamodelX/Red/Model.pm6:85-87 — The column-values method maps via %!attr-to-column{.name} as the hash key. Consider documenting this returns DB column names (not attribute names), since the distinction may surprise users.

📊 Stats

Metric	Value
Files changed	4
Lines added	+64
Lines removed	−3
New tests	`t/20-in-pg.t` (48 lines, PG-only, env-gated)
Security scan	Clean — no secrets, debuggers, or merge conflicts

Reviewed by Hermes Agent

hermes-fco · 2026-06-22T14:24:35Z

Code Review — Some small tweaks to Red

Verdict: Comment (0 critical, 1 warning, 2 suggestions)

PR adds column-values method, fixes $data immutability in creates, improves IN placeholder via self.wildcard, and registers Pg types (text/json/jsonb). 4 files, +64/−3.

✅ Looks Good

column-values method — clean, concise implementation. Already landed in master's .rakumod migration.
self.wildcard replacement — correct fix; already landed in master at CommonSQL.rakumod:739.
%data-copy workaround — sensible defense against immutable Map exceptions during create+id-filter.

⚠️ Warnings

t/20-in-pg.t:19 — $GLOBAL::RED-DB should be $*RED-DB. $GLOBAL:: bypasses the dynamic variable lookup and will silently point at the wrong DB (or Nil) in any scope where $*RED-DB is locally overridden. This is a bug.
```
# ❌
$GLOBAL::RED-DB = database "Pg", :dbname($*RED-PG-TEST-DB);
# ✅
$*RED-DB = database "Pg", :dbname($*RED-PG-TEST-DB);
```

💡 Suggestions

Repo has migrated from .pm6 to .rakumod — this PR targets files that no longer exist on master (Model.pm6, CommonSQL.pm6, Pg.pm6). The column-values and self.wildcard changes have already been incorporated into the .rakumod equivalents. The remaining Pg type-by-name additions could be ported to Pg.rakumod if still needed.
t/20-in-pg.t:8 — plan 2 declared at top, but skip-rest 2 at line 9 means the plan count is technically wrong when the env var is missing (0 tests run, plan expects 2). Consider plan 2 unless $skip or moving plan after the guard.

📊 Stats

Metric	Value
Files changed	4
Lines added	+64
Lines removed	−3
New tests	1 test file (`t/20-in-pg.t`)
Security scan	Clean

Reviewed by Hermes Agent

xinminglai added 3 commits November 2, 2019 20:42

Add column-values method to MetamodelX::Red::Model

7b1420b

Additional types text/json/jsonb info for Driver::Pg

3cdb36b

Fix In operation for Driver::Pg

18038bf

FCO requested changes Nov 2, 2019

View reviewed changes

xinminglai added 5 commits November 24, 2019 07:50

Merge remote-tracking branch 'fco/master'

c61a81c

Merge remote-tracking branch 'fco/master'

8e1055f

Use self.wildcard to implement the In with @array values.

cd6984a

Use data-copy to prevent modify of immutable exception.

d3b0cab

FCO reviewed Jan 17, 2020

View reviewed changes

Uh oh!

Conversation

xinminglai commented Nov 2, 2019

Uh oh!

FCO commented Nov 2, 2019

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!

hermes-fco commented Jun 22, 2026

Code Review — Some small tweaks to Red.

✅ Looks Good

⚠️ Warnings

💡 Suggestions

📊 Stats

Uh oh!

hermes-fco commented Jun 22, 2026

Code Review — Some small tweaks to Red

✅ Looks Good

⚠️ Warnings

💡 Suggestions

📊 Stats

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants