microsoft / DacFx

DacFx, SqlPackage, and other SQL development libraries enable declarative database development and database portability across SQL versions and environments. Share feedback here on dacpacs, bacpacs, and SQL projects.
https://aka.ms/sqlpackage-ref
MIT License
343 stars 20 forks source link

ScriptDom parser for Sql Server 2022 fails to parse IGNORE NULLS clause #133

Closed clement911 closed 1 year ago

clement911 commented 2 years ago

Steps to Reproduce:

  1. Create a new C# Console Project
  2. Add latest Nuget package for Microsoft.SqlServer.DacFx (160.6266.0-preview)
  3. Run code below
using Microsoft.SqlServer.TransactSql.ScriptDom;

var parser = new TSql160Parser(true); //sql server 2022 parser
IList<ParseError> errors;

parser.ParseExpression(new StringReader("LAST_VALUE(Measure) OVER()"), out errors);
//no parsing errors

parser.ParseExpression(new StringReader("LAST_VALUE(Measure) IGNORE NULLS OVER()"), out errors);
//parsing error! "Incorrect syntax near IGNORE." This should parse succesfully

Console.ReadLine();

Did this occur in prior versions? If not - which version(s) did it work in? Not applicable because the Sql Server 2022 parser (TSQL 160 didn't exist in prior versions).

Documentation about the support for IGNORE NULLS / RESPECT NULLS in Sql Server 2022 is here: https://docs.microsoft.com/en-us/sql/t-sql/functions/last-value-transact-sql?view=sql-server-ver16#-ignore-nulls--respect-nulls-

dzsquared commented 2 years ago

similar bucket to #105

clement911 commented 2 years ago

@dzsquared any updates on open sourcing the ScriptDom lib? This kind of bug really gets us stuck and we cannot implement our DB tool for sql azure and sql server 2022 until a fix is released, because scripts that are valid don't parse. If we could contribute, we could send a PR to fix these issues.

clement911 commented 2 years ago

Please open up the code so that we can contribute and fix bugs.

zijchen commented 2 years ago

@clement911 - ScriptDom is one of the projects we plan to open-source first. In the meantime, support for this syntax should be in our latest 160.6324.0-preview release. Please give it a try!

clement911 commented 1 year ago

@zijchen I can confirm that parsing of the expression above now works on version 161.6337.0-preview.

However, I find the object model weird. Why is the IgnoreRespectNulls property a list of Identifiers?

public IList<Identifier> IgnoreRespectNulls { get; }

We would expect a proper Fragment class IgnoreRespectNullsClause or maybe a simple a simple enum

Maybe something along the following lines?

public enum IgnoreRespectNulls
{
  NotSpecified,
  IgnoreNulls,
  RespectNulls
}

p.s.: I also tested a long standing bug causing the parser to fail to parse nested IIF calls. I'm pleased to see this appears to be fixed!

zijchen commented 1 year ago

@namangupta211 FYI, maybe we can keep this issue open as an enhancement to implement the IgnoreRespectNulls as its own clause instead of a list of identifiers

namangupta211 commented 1 year ago

Hi Zi, yes, we can do this.

Get Outlook for iOShttps://aka.ms/o0ukef


From: Z Chen @.> Sent: Wednesday, October 26, 2022 8:28:28 PM To: microsoft/DacFx @.> Cc: Naman Gupta @.>; Mention @.> Subject: Re: [microsoft/DacFx] ScriptDom parser for Sql Server 2022 fails to parse IGNORE NULLS clause (Issue #133)

@namangupta211https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnamangupta211&data=05%7C01%7Cnamangupta%40microsoft.com%7C0a6ed614d50145ace62508dab7628a9f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638023931132816667%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Gw%2F0lRqIWyXfS8bGdHhPFihFl0%2FCRfETcwrGPZEbh0U%3D&reserved=0 FYI, maybe we can keep this issue open as an enhancement to implement the IgnoreRespectNulls as its own clause instead of a list of identifiers

— Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FDacFx%2Fissues%2F133%23issuecomment-1292183017&data=05%7C01%7Cnamangupta%40microsoft.com%7C0a6ed614d50145ace62508dab7628a9f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638023931132816667%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5oJJl9fll%2BnFuAlRIpi7MiyTbTDaONOh7kAPSlTaCi8%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FA2XYWH6TREOBMDNRQI2J5HLWFFBJJANCNFSM57U5GISA&data=05%7C01%7Cnamangupta%40microsoft.com%7C0a6ed614d50145ace62508dab7628a9f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638023931132816667%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kttfHyK9NBQZ7hlcXNSAu%2BuoLyTzHgd3E2JiSFA%2FRaE%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

@llali I see that you have closed. Was the enhancement mentioned above implemented?

llali commented 1 year ago

@namangupta211 do you want to create a separate task for the enhancement mentioned above? The bug was originally created for the parser bug so that's why I closed it since it's fixed

clement911 commented 1 year ago

@namangupta211 ?

llali commented 1 year ago

@clement911 I added the feature request here: https://github.com/microsoft/DacFx/issues/181 please add any missing details. Thanks

clement911 commented 1 year ago

Thanks @llali

@zijchen any eta on open sourcing?

clement911 commented 1 year ago

I see a new stable version of the package was released yet this is still not fixed