Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add MapColumns method #6901

Merged
merged 3 commits into from
Jun 24, 2024
Merged

feat: add MapColumns method #6901

merged 3 commits into from
Jun 24, 2024

Conversation

molon
Copy link
Contributor

@molon molon commented Mar 14, 2024

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Add MapColumns method: modify the column names in the query results to facilitate align to the corresponding structural fields

User Case Description

I recently used gorm to do aggregation queries based on timescaledb. For example, the following query:

SELECT
    symbol,
    '5min' AS timeframe,
    TRUE AS adj,
    time_bucket('5 minute', t, 'America/New_York') AS bucket,
    time_bucket('5 minute', t, 'America/New_York') AS t,
    FIRST(o, t) AS o,
    MAX(h) AS h,
    MIN(l) AS l,
    LAST(c, t) AS c,
    SUM(v) AS v,
    SUM(n) AS n,
    SUM(v*c)/SUM(v) AS vw
FROM
    market.candle
WHERE
    adj = TRUE AND
    timeframe = '1min' AND 
    symbol = 'AAPL' AND
    time_bucket('5 minute', t, 'America/New_York') > '2023-03-03 17:01:00+08'
GROUP BY
    symbol, bucket
ORDER BY
    symbol, bucket ASC
LIMIT 10000;
bucketWidth := timeFrameToBucketWidth(req.TimeFrame)
db = db.Select(`
	symbol,
	? AS timeframe,
	TRUE AS adj,
	time_bucket(?, t, ?) AS bucket,
	time_bucket(?, t, ?) AS t,
	FIRST(o, t) AS o,
	MAX(h) AS h,
	MIN(l) AS l,
	LAST(c, t) AS c,
	SUM(v) AS v,
	SUM(n) AS n,
	SUM(v*c)/SUM(v) AS vw
`, req.TimeFrame.String(), bucketWidth, s.locationName, bucketWidth, s.locationName)
db = db.Where(`symbol IN ? AND timeframe = '1min' AND adj = ?`, req.Symbols, req.Adjustment)
if !req.Start.IsZero() {
	db = db.Where(fmt.Sprintf("time_bucket(?, t, ?) %s ?", startCompare), bucketWidth, s.locationName, req.Start)
}
if !req.End.IsZero() {
	db = db.Where("time_bucket(?, t, ?) < ?", bucketWidth, s.locationName, req.End)
}
db = db.Group("symbol, bucket")
db = db.Order("symbol ASC, bucket ASC")

The result will be an error ERROR: column "candle.t" must appear in the GROUP BY clause or be used in an aggregate function (SQLSTATE 42803).
However, I cannot put t in Group, which will affect the correct result. So I tried using db = db.Group("symbol, time_bucket('5 minute', t, 'America/New_York')") which unfortunately returns the same error.
So I can only give up returning the t column in the result set, but then I need to customize a nested structure to supplement a bucket column, and then manually assign it to MarketCandle.T. This is very inconvenient.
So I added the MapColumns method. I think this may be a requirement that also exists in other scenarios, especially when GROUP BY is used.

@jinzhu
Copy link
Member

jinzhu commented Apr 26, 2024

Hi @molon

Could you please include some tests to ensure it functions correctly?

@molon
Copy link
Contributor Author

molon commented May 9, 2024

Hi @molon

Could you please include some tests to ensure it functions correctly?

When I tried to write test cases, I found that my current implementation was unable to meet the scenario where Embedded existed.
Because Embedded will cause duplicate column names to appear.

Do you have any suggestions for this?

@jinzhu
Copy link
Member

jinzhu commented Jun 17, 2024

Hi @molon
Could you please include some tests to ensure it functions correctly?

When I tried to write test cases, I found that my current implementation was unable to meet the scenario where Embedded existed. Because Embedded will cause duplicate column names to appear.

Do you have any suggestions for this?

Embedded fields do not necessarily lead to duplicate field names, but I believe we should be able to disregard such issues, right?

@molon
Copy link
Contributor Author

molon commented Jun 22, 2024

Agreed, already added it.

@jinzhu jinzhu merged commit 11c4331 into go-gorm:master Jun 24, 2024
34 checks passed
@shayan0v0n
Copy link

thanks for adding this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants