DacFx icon indicating copy to clipboard operation
DacFx copied to clipboard

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

Open clement911 opened this issue 1 year ago • 4 comments

  • SqlPackage or DacFx Version: 160.6266.0-preview
  • .NET Framework (Windows-only) or .NET Core: .NET 6.0 (Core)
  • Environment (local platform and source/target platforms): N/A

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-

clement911 avatar Aug 26 '22 00:08 clement911

similar bucket to #105

dzsquared avatar Aug 26 '22 17:08 dzsquared

@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 avatar Sep 08 '22 11:09 clement911

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

clement911 avatar Oct 06 '22 06:10 clement911

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

zijchen avatar Oct 18 '22 18:10 zijchen

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

clement911 avatar Oct 26 '22 06:10 clement911

@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

zijchen avatar Oct 26 '22 14:10 zijchen

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: @.***>

namangupta211 avatar Oct 26 '22 15:10 namangupta211

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

clement911 avatar Nov 09 '22 21:11 clement911

@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

llali avatar Nov 09 '22 21:11 llali

@namangupta211 ?

clement911 avatar Nov 16 '22 05:11 clement911

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

llali avatar Nov 16 '22 15:11 llali

Thanks @llali

@zijchen any eta on open sourcing?

clement911 avatar Nov 16 '22 19:11 clement911

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

clement911 avatar Nov 17 '22 21:11 clement911