fix: don't strip SQL comments in Explore - 2nd try

[mistercrunch]

Here I'm trying to recreate the issue that led to revert #28363 in #28567, likely some sort of trailing comment of some kind. This is DRAFT for now until I can reproduce the issue, write a covering test for it, and address it.

First attempt at creating a problematic virtual dataset was the following ->

[mistercrunch]

@michael-s-molina could use a failing query or more input on the pattern identified

[michael-s-molina]

/testenv up

[michael-s-molina]

@michael-s-molina could use a failing query or more input on the pattern identified

/* Comment */
select
	r.column_a as column_1 ,
	r.column_b as column_2 ,
	r.ds as ds_removal ,
	a.column_a is not null as column_3 ,
	a.ds as ds_appeal_attempt ,
	lr.column_c as column_4 ,
	r.column_d as column_5 ,
	r.column_e as column_6 ,
	r.column_f as column_7 ,
	r.column_g as column_8 ,
	r.column_h as column_9 ,
	r.column_i as column_10 ,
	active.column_j as column_11
from
	schema_a.table_a r
join schema_a.table_b lr on
	r.column_a = lr.column_a
	and r.ds >= lr.column_b_start
	and r.ds < lr.column_b_end
	and lr.ds = (
	select
		MAX(ds)
	from
		schema_a.table_b)
join schema_b.table_a active on
	r.column_a = cast(active.column_a as VARCHAR)
	and active.ds = '{{presto.latest_partition(' schema_b.table_a')}}'
left join schema_a.table_a a on
	r.column_a = a.column_a
	and a.column_k = 'quality'
	and a.column_l = 1
	and a.ds >= r.ds
	and r.ds >= '2023-04-03'
where
	r.column_k = 'quality'
	and r.column_l = 1
	and r.ds >= '2023-04-03'
	and r.column_m = 'points' -- comment

[michael-s-molina]

with table_a as (
select
	column_a as interaction_id,
	trim_array(column_a,
	1 + column_b['column_c']) as column_d,
	(cardinality(column_a) -1 - column_b['column_c']) as column_e,
	column_f as column_g,
	column_h as column_i,
	column_j as column_k,
	column_l as column_m,
	column_n as column_o,
	column_p as column_q,
	column_r as column_s,
	column_t as column_u,
	ds
from
	schema_a.table_a
where
	cardinality(column_a) > 1
	and column_s = 'PHONE'
	and column_v = 'INBOUND'
	and (column_b['column_c'] = 0
		or (column_b['column_c'] = 1
			and column_b['column_w'] = 3)) )
select
	c.column_x as column_y,
	c.column_z as column_aa,
	c.column_ab as column_ac,
	c.column_g as column_ad,
	c.column_ae as column_af,
	c.column_ag as column_ah,
	c.column_ai as column_aj,
	a.interaction_id,
	a.column_b,
	a.column_c,
	column_e,
	a.column_d as column_aj,
	a.column_f as column_ak,
	d.column_al,
	cast(from_iso8601_timestamp(d.column_r) as TIMESTAMP) as column_am,
	cast(from_iso8601_timestamp(d.column_s) as TIMESTAMP) as column_an,
	date_diff('second',
	cast(from_iso8601_timestamp(d.column_r) as TIMESTAMP),
	cast(from_iso8601_timestamp(d.column_s) as TIMESTAMP)) as column_ao,
	d.ds,
	a.column_g,
	a.column_i as column_ap,
	a.ds as column_aq,
	t.column_as
from
	table_a a
cross join unnest(a.column_d) t(column_as)
join schema_a.table_b c on
	c.column_a = t.column_as
left join schema_b.table_c d on
	c.column_at = d.column_at
where
	c.column_au
	and d.column_av = 'NOT_RESPONDING'
	and c.ds = cast(cast('{{ presto.latest_partition(' schema_a.table_b') }}' as DATE) as VARCHAR)
	and a.ds = cast(cast('{{ presto.latest_partition(' schema_a.table_a') }}' as DATE) as VARCHAR)
	--AND d.ds = CAST(CAST('{{ presto.latest_partition('schema_b.table_c') }}' AS DATE) AS VARCHAR) 
	AND CAST(from_iso8601_timestamp(d.column_r) AS TIMESTAMP) between a.column_d and a.column_e

[michael-s-molina]

select *
from (
   select rank() over (partition by table_a order by updated_at desc) as rnk
   , table_a as metadata_table_a
   , cast(date_trunc('day', from_unixtime(cast(json_extract(metadata, '$.a.b') as double))) as date)
   --, CAST(json_extract(metadata,'$.a.c') as VARCHAR) x
   , cast(json_extract(metadata, '$.a.d.0.f') as varchar) region_0
   from (
	select rank() over (partition by table_a order by if(metadata is not null, 1, 2)) as rnk_null
            ,  *
	from schema_a.table_b
	where ds = '{{trino.latest_partition(' schema_a.table_b') }}' 
             and type = 'abc' and substr(deleted_at, 1, 4) = '9999'
   )
   where rnk_null = 1

[mistercrunch]

Reading the examples, pattern seems to be simply some dash-dash comment anywhere in the query. Hoping I can recreate it on top of the example database. Will keep you posted here.

[mistercrunch]

@michael-s-molina this is the only case where I was able to reproduce a clear failure given the previous logic. Basically this particular test case fails on the previous PR that we merge/reverted, but passes here.

Happy to add more test cases here if you'd like to build more confidence before merging this.

[michael-s-molina]

@mistercrunch I cherry-picked your latest changes into our staging environment and was able to confirm the issue was fixed. Thanks for the investigation and added tests.

[mistercrunch]

This is the fix compared to the original PR, basically forcing a line break after the FROM clause subquery.
"""

-- BEFORE, notice how the `) AS` is in-comment
{...}
 FROM cte --COMMENT) AS virtual_table GROUP BY col1, col2

-- AFTER, notice the line break after the comment, allowing proper aliasing of the subquery
{...}
 FROM cte --COMMENT
) AS virtual_table GROUP BY col1, col2

[michael-s-molina]

My only suggestion would be to add a comment in the code explaining why do we need the line break.

[john-bodley]

@mistercrunch just to verify, does this logic work for /* ... */ multiline comments?

[mistercrunch]

i noticed (again) that superset/connectors/sqla/models.py and superset/models/helpers.py have a fair amount of copy/pasta/dup-logic. I'll try and fix that in a future PR.

[michael-s-molina]

Not sure if this was intended as part of this PR 🤷🏼‍♂️

[mistercrunch]

Yes I should have added a note, but my new unit test test_virtual_dataset_with_comments uses "in-a-subquery CTE", which apparently isn't supported in mysql 5.7, failing that one test against old mysql. That CTE is one way that I was able to reproduce some of the issues from the previous reverted PR.

Given the wikipedia page, 5.7 isn't supported anymore and 8.0 is the next release. I'm unclear about the gap but might have considered doing a smaller bump if there was an option. Bumping to 8.0 seems like the right move.

[rusackas]

Code owner stamp of approval :)