Fix: include t.* columns in RECORDSET results (fixes #2070) #2153

shhreya13 · 2025-11-03T14:12:09Z

This PR fixes issue #2070, where
RECORDSET OF SELECT t.*, computed_column FROM ? t
returned only the computed column (rn) in the columns metadata, omitting those from t.*.

🧠 Summary of Fix

Ensures that all columns from t.* are correctly included in the columns array
when additional computed columns are present.
Updated logic in src/40select.js to merge both sets of columns.

🧪 Test

Verified manually using:

const data = [{a:1,b:10}, {a:2,b:20}, {a:1,b:30}];
const res = alasql('RECORDSET OF SELECT t.*, 1 as rn FROM ? t', [data]);
console.log(res.columns.map(c => c.columnid));
// Expected output: ['a', 'b', 'rn']

mathiasrw

Way too many things changed in this PR. But lets lets work with it now that you have created it.

You shuold run yarn test locally to confirm things are working. 4 tests are broken.

The PR also needs to add new test file named test2070 that verifies that the problem in #2070 is not accidentally reintroduced in the future.

mathiasrw · 2025-11-03T22:41:41Z

src/40select.js

 }
+
+// ✅ Custom RECORDSET class
+yy.Recordset = class Recordset extends yy.Select {


Looks like new code.

When/why are we using this?

mathiasrw · 2025-11-03T22:41:58Z

src/40select.js

 	return res;
 }
+
+// ✅ Custom RECORDSET class


Please remove comments. Make the code speak.

mathiasrw · 2025-11-03T22:42:11Z

src/40select.js

+			}, {});
+
+		case 'RECORDSET': {
+			// ✅ FIX: include all keys from t.* + computed columns


Please remove coments. Let. the code. speak.

mathiasrw · 2025-11-03T22:44:21Z

src/40select.js

+			const keyIndex = columns?.[0]?.columnid || Object.keys(res[0])[0];
+			const valIndex = columns?.[1]?.columnid || Object.keys(res[0])[1];
+			return res.reduce((acc, row) => {
+				acc[row[keyIndex]] = row[valIndex];


An improvement. Nice.

But not really related to the PR. In the future please make a seperate PR with things not related to the PR. Lets just keep it for now.

mathiasrw · 2025-11-03T22:46:17Z

src/40select.js

-
-			return ar;
+			const key = columns?.[0]?.columnid || Object.keys(res[0])[0];
+			return res.map(r => r[key]);


Nice with an improvement. But not really related to the PR. In the future please make a seperate PR with things not related to the PR. Lets just keep it for now.

Please use ?? instead of ||

mathiasrw · 2025-11-03T23:15:57Z

src/40select.js

-
 		query.defcols = this.compileDefCols(query, databaseid);
-
-		// 1. Compile FROM clause


Please keep comment

mathiasrw · 2025-11-03T23:16:23Z

src/40select.js

 		query.modifier = this.modifier;
-
 		query.database = db;
-		// 0. Precompile whereexists


Please leave comment

mathiasrw · 2025-11-03T23:16:48Z

src/40select.js

+		);
 	}

-	// Compile SELECT statement


Please keep comment

mathiasrw · 2025-11-03T23:20:23Z

src/40select.js

-			s += ' UNION ALL ' + (this.corresponding ? 'CORRESPONDING ' : '') + this.unionall.toString();
-		}
-		if (this.except) {
+		if (this.unionall)


Please avoid multi line if statement without brackets (even if there is only one statement)

mathiasrw · 2025-11-03T23:21:37Z

src/40select.js

+				.map(jn => {
+					let ss = ' ';
+					if (jn.joinmode) ss += jn.joinmode + ' ';
+					if (jn.table) ss += 'JOIN ' + jn.table.toString();


Please always use {...} when involving an else

mathiasrw · 2025-11-05T13:32:18Z

Please run yarn format-all and push again.

shhreya13 and others added 3 commits November 3, 2025 19:39

Fix: include t.* columns in RECORDSET results (fixes AlaSQL#2070)

3fd6b44

Resolve yarn.lock conflict by keeping upstream version

da7e6c7

Merge branch 'develop' into fix/recordset-columns

bdab389

mathiasrw requested changes Nov 3, 2025

View reviewed changes

mathiasrw and others added 3 commits November 4, 2025 13:22

Merge branch 'develop' into fix/recordset-columns

ff549d1

Merge branch 'develop' into fix/recordset-columns

d36c3f2

Merge branch 'develop' into fix/recordset-columns

138d15f


		query.defcols = this.compileDefCols(query, databaseid);

		// 1. Compile FROM clause

Fix: include t.* columns in RECORDSET results (fixes #2070) #2153

Are you sure you want to change the base?

Fix: include t.* columns in RECORDSET results (fixes #2070) #2153

Uh oh!

Conversation

shhreya13 commented Nov 3, 2025

🧠 Summary of Fix

🧪 Test

Uh oh!

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

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!

mathiasrw commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants