[SPARK-31257][SQL] Unify create table syntax (WIP)

What changes were proposed in this pull request?

Unify the create table syntax in the parser
Add SerdeInfo and external boolean to statement plans

Why are the changes needed?

Current behavior is confusing.
A way to pass the Hive create options to DSv2 is needed for a Hive source.

Does this PR introduce any user-facing change?
Not by default, but v2 sources will be able to handle STORED AS and other Hive clauses.
How was this patch tested?
Existing tests for current behavior, new parser tests.

Test build #120376 has started for PR 28026 at commit 0522c93.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25085/
Test PASSed.

Could you please pick up SPARK-31257 instead as the JIRA issue explains better for the "situation" we are in? It's still good to keep the PR title as it is, as we tend to explain the "problem/issue" in JIRA and explain "how" to deal with it in PR.

Test build #120376 has finished for PR 28026 at commit 0522c93.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120376/
Test FAILed.

I understand that we want to make Hive implements the v2 API eventually, but can we focus on syntax unification right now? Let's not change the behavior, for example, EXTERNAL should still be disallowed when creating native data source tables.

I don't know who marked comments as resolved so please correct me if I'm wrong, but assuming that @cloud-fan comments to the resolved comment, it doesn't seem @cloud-fan marked comments as resolved.
Could we make sure comments are marked as resolved only when both agree?

Same here, please describe TODO list for this PR to remove WIP.
If I skimmed the code correctly, this "requires" end users to add USING hive even they add Hive create table specific clause. Do I understand correctly?
It's OK if that's one of TODO - we should concern about documentation to make clear which clause changes the provider. (Personally I'm not in favor of this, but otherwise why not just add HIVE to second syntax as both are requiring end users to change their query and adding HIVE is clearer?)
If the change is intentional, that's my favor but I may concern about removing compatibility flag, as this will force end users to migrate their query in any way.

I don't know who marked comments as resolved

That was me because I thought they were fairly straight-forward and definitive answers. We can still discuss them more since that wasn't the case.

If I skimmed the code correctly, this "requires" end users to add USING hive even they add Hive create table specific clause.

That's not quite right.
The parser now creates a CreateTableStatement that contains everything that was parsed from the SQL string, with minimal interpretation. That is what the user requested, assuming that request was well-formed -- meaning that it doesn't have duplicate clauses or mixed partition fields.
The next step is to convert that statement to a plan. At the moment, this only converts to v2 plans because we no longer use v1 and this was ported from our internal version. The interpretation of that statement for v1 -- converting to v1 CreateTable -- has not yet been added. I plan to implement that like this:

If provider is present from USING, set the CatalogTable provider and validate that no SerdeInfo is set.
If SerdeInfo is present, validate that provider was not set and use a Hive CatalogTable.
If neither SerdeInfo nor provider is set, use SQLConf.LEGACY_CREATE_HIVE_TABLE_BY_DEFAULT_ENABLED or similar setting to set a default provider or default serde properties.

Speaking of well-formed, I think it might be reasonable to ensure that either a provider or serde info is present, but not both. That would raise an exception if both USING and STORED AS or SERDE clauses were used. I think that's probably reasonable. In that case, we could skip the checks above. What do you think?
Coming back to the current implementation for the v2 interface: Spark is passing through everything that was parsed because Spark doesn't know what kind of table the catalog is going to create. The v2 catalog that we use can create Hive tables, Spark datasource tables, and Iceberg tables, so all of the information needs to be passed through -- again, with the requirement that the SQL was well-formed.

Spark hides the EXTERNAL TABLE concept from users

From Hive's documentation for managed tables:

The default location can be overridden by the location property during table creation

Hive allows EXTERNAL with a custom LOCATION, so I don't think that Spark should prevent this from being passed to catalogs. A catalog may choose to allow this. Spark can prevent its internal catalog from creating such tables, but it can't prevent them from existing.
Since this is a valid configuration for a table, it must be possible to pass this through to a v2 catalog.

Thanks for the detailed answer. Really appreciated.
I misunderstood something as you commented, but it seems to come from something which is not addressed yet and the plan was not shared. Without sharing the detailed plan (TODO) I wonder the review can be done correctly. Maybe we'd be better to wait for you to make more progress?
Btw, this change doesn't seem to be trivial but PR description has shortage of information - one line per a major change. I guess we tend to describe at least how it works in high level on the PR description so that commit message itself becomes informative. We may want to get it once the PR is ready to merge.
Back to the comment, your plan looks promising to me, but I'm not an expert of DSv2 (what I originally pointed out was ambiguous parser syntaxes, not about DSv2) so I feel I'm not qualified.

Speaking of well-formed, I think it might be reasonable to ensure that either a provider or serde info is present, but not both. That would raise an exception if both USING and STORED AS or SERDE clauses were used. I think that's probably reasonable. In that case, we could skip the checks above. What do you think?

Would we like to disallow STORED AS/SERDE clauses if the provider is specified to Hive via USING clause? That would be still OK if we provide good error message and guide alternatives (either doc or error message).

Test build #120620 has started for PR 28026 at commit 12a17fc.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25322/
Test PASSed.

Test build #120620 has finished for PR 28026 at commit 12a17fc.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120620/
Test FAILed.

Hive allows EXTERNAL with a custom LOCATION, so I don't think that Spark should prevent this from being passed to catalogs.

This is similar to the table option/property discussion. Hive has more features (more flexible parser) than Spark, but it doesn't mean Spark needs to accept them all.
When we introduced the Spark native CREATE TABLE syntax, we evaluated the Hive syntax and decided to forbid the EXTERNAL keyword. Hive requires LOCATION to be specified if EXTERNAL exists, but also allows LOCATION to be specified even there is no EXTERNAL. So the missing feature is: creating a managed table with a custom location. This feature is weird and actually Hive metatore gives a warning message if the client creates managed tables with custom location, which implies that Hive doesn't recommend using this feature as well.
I think we shouldn't put EXTERNAL into v2 API and we should probably forbid EXTERNAL for hive tables as well eventually, to be consistent with Spark native tables.

Hive has more features (more flexible parser) than Spark, but it doesn't mean Spark needs to accept them all.

I don't think Spark needs to make a choice on this for the v2 API. It is up to the catalog what features of a table format are useful or not. Because Spark supports parsing EXTERNAL, it makes no sense to arbitrarily suppress it. That requires more code in Spark that is specific to what is now a responsibility delegated to a catalog. It breaks the abstraction.

EXTERNAL is never supported in Spark native tables, and it's intentional. At least at that time, we thought EXTERNAL is a bad feature and Spark shouldn't have it. Currently Spark supports it only for Hive compatibility, and it's an option to make the v2 API simpler by dropping EXTERNAL completely and sacrificing Hive compatibility.

What you're suggesting is to maintain compatibility with Hive SQL, but arbitrarily limit what a source connector can do.
How about this: users shouldn't be allowed to use the | character as the delimiter for Hive's delimited format in sequence files. Let's add special code to Spark to check for that case and fail it before passing everything to the catalog.
Hopefully you see my point: making decisions about what catalogs should support is not a reasonable thing for Spark to do. Spark should reject what is not well-formed, but should not decide details for specific sources.
And to be clear, if you want to do this in Spark's built-in Hive catalog/connector then that's fine with me. But this kind of decision should be made by the catalog, not in the parser or in logical plans.

You made a point that the same problem occurs for STORED AS/ROW FORMAT DELIMITED. How are we going to support them in the v2 catalog?
IIRC the v2 catalog API was design based on the native CREATE TABLE syntax, i.e. provider, table properties, etc. These Hive specific syntaxes were not considered.

Hive syntax was considered. Anything specific to a source will be passed as table properties.
In this PR, these are passed as follows:

STORED AS formatName -> hive.stored-as=formatName
INPUTFORMAT formatClass -> hive.input-format=formatClass
OUTPUTFORMAT formatClass -> hive.output-format=formatClass
SERDE serdeClass -> hive.serde=serdeClass
SERDEPROPERTIES ('key'='value') -> option.key=value

Again, the only thing that Spark should be doing here is ensuring the data is well-formed and then passing it directly to the source.

Build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25357/
Test PASSed.

@cloud-fan, I've implemented the conversions to v1 plans and updated the tests like the other commands that we moved to use statements. Tests from DDLParserSuite are now in PlanResolutionSuite. I also needed to move a create test from SparkSqlParserSuite into PlanResolutionSuite for the same reason.
The create test in SparkSqlParserSuite highlighted an existing problem with spark.sql.legacy.createHiveTableByDefault.enabled. With that table property disabled (default to USING), Hive was still used to create a table because Hive's PARTITION BY syntax was used. I think this is very confusing behavior! (@HeartSaVioR probably agrees.)
Because of this problem, I think we should either roll back the patch to make USING the default in 3.0 since it is not applied everywhere. That, or get this syntax unification into 3.0.

Test build #120658 has started for PR 28026 at commit 8bf400d.

Test build #120658 has finished for PR 28026 at commit 8bf400d.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

Build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120658/
Test FAILed.

@HeartSaVioR, here's some additional info to give you context.
When we convert plans to v2, we alter the parser to produce a Statement plan that contains what was parsed, without modification. The reason is that the v1 plans put a lot of logic about what was allowed in the parser itself, which required the parser to be broken across spark-catalyst and spark-sql modules and was difficult to maintain. Now, logic about what is allowed is in the analyzer.
Statement plans are converted to either the exact same v1 logical plan as before, or a v2 plan. To validate that we are producing the exact same logical plans, we move tests from the spark-sql DDLParserSuite (only runs the parser) into PlanResolutionSuite that runs the parser and conversion rules. These tests should be nearly identical to the original and are as close to a copy & paste as possible to be easy to verify. I just completed this step, which is why tests should be passing.
In addition, we add tests to spark-catalyst's DDLParserSuite that test SQL to Statement conversion. These are the last tests that I need to add for this PR.
Other than that, I need to rebase on master because there are conflicts.

Test build #120659 has started for PR 28026 at commit 50019e6.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25358/
Test PASSed.

Test build #120659 has finished for PR 28026 at commit 50019e6.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120659/
Test FAILed.

The create test in SparkSqlParserSuite highlighted an existing problem with spark.sql.legacy.createHiveTableByDefault.enabled. With that table property disabled (default to USING), Hive was still used to create a table because Hive's PARTITION BY syntax was used. I think this is very confusing behavior! (@HeartSaVioR probably agrees.)

Sure, I totally agree. That's one of the issue I mentioned in mail thread, refer below link:
https://lists.apache.org/thread.html/ra2aa31ed7cbd4e34d3504adc97cae1301cc249bfb8f95565b808b0cb%40<dev.spark.apache.org>
It's wrong if someone simply thinks that if some words (ROW FORMAT, STORED AS) exist then Hive create table will be used. There're more differences on details, including the point @rdblue pointed out here. The issue didn't exist before, as USING clause plays as a marker to distinguish two syntaxes.

Because of this problem, I think we should either roll back the patch to make USING the default in 3.0 since it is not applied everywhere. That, or get this syntax unification into 3.0.

That's in line with my proposal as well. The ideal approach would be reverting SPARK-30098 in Spark 3.0 and make it correct with syntax unification in further version. I've also proposed some alternatives (add a marker, turn on legacy config by default) but haven't heard any feedback.

OK let's flip the value of spark.sql.legacy.createHiveTableByDefault.enabled for 3.0, so we have more time to discuss this PR.
@Ngone51 can you open a PR to flip the config value?

Maybe we also need to change two "create table" pages and migration guide page as well  regardless of the approach, since we are changing the default behavior.

@Ngone51 can you open a PR to flip the config value?

Maybe we also need to change two "create table" pages and migration guide page as well regardless of the approach, since we are changing the default behavior.

Ok.

@cloud-fan, @Ngone51: to be clear, I think that the PR that added that config property should be reverted if this one doesn't get into 3.0. Allowing people to turn on this confusing behavior is a bad idea.
I think we should fix this, so that we don't need to wait until 4.0 for the behavior changes. After this PR, datasource tables will support Hive's PARTITION BY syntax as an alternative to partition expressions. I think it would be best to get all those changes into 3.0.

Test build #120683 has started for PR 28026 at commit d7500c8.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25382/
Test PASSed.

Test build #120683 has finished for PR 28026 at commit d7500c8.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120683/
Test FAILed.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25383/
Test PASSed.

Test build #120684 has started for PR 28026 at commit 2484059.

Test build #120684 has finished for PR 28026 at commit 2484059.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120684/
Test FAILed.

Test build #120686 has started for PR 28026 at commit dc1b761.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25385/
Test PASSed.

Test build #120686 has finished for PR 28026 at commit dc1b761.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120686/
Test FAILed.

I agree turning on the legacy config is just a workaround which I will never recommend end users to turn off. I expect I would fail to explain to end users why this query creates parquet provider but other query creates Hive provider, and I guess that's not only me. (You may be able to understand although such complication but that's different story if you have to make others understand.)
If I remember correctly, the Spark community practice for similar case has been "revert". There's no point of keep the commit be live as it's known to be bad and we will have better patch in near future. It only makes sense if someone disagrees the commit is bad; is that a case? Or do we want to let some individuals/teams/vendors to manage it with risks?
ps. Honestly, I feel really odd we allow turning on legacy config by default - not for this issue but for other issues - end users may never indicate the fix, although there should be the valid reason the fix is landed.

I tend to agree it would be ideal to get this patch in Spark 3.0.0, but then this patch will become a blocker for Spark 3.0.0, which means (at least for me) this PR should be handled as urgent manner. Reverting the commit would leave us in a good shape (not improved but not worse) which basically means this issue would become a type of "improvement" and never be a blocker.
I agree the change may need to be introduced in Spark 4.0 due to semver (the main point of worry), but it might be possible the community decides to ship it in minor version if the community thinks the benefit clearly outweighs the semantic of semver.
It would be nice to sync up with release manager for Spark 3.0.0 to weigh the benefit on marking this as blocker for Spark 3.0.0. cc. @rxin

Test build #120699 has started for PR 28026 at commit 5e8f5d0.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25397/
Test PASSed.

The last test results were mostly tests that failed because of the syntax differences. Those tests used SQL syntax that couldn't be parsed by the DataSource CREATE rule, but also didn't specify either USING or Hive storage options. That shows that there were at least a lot of test queries silently affected by this ambiguity that were uncovered by unifying the syntax.
To avoid too many changes in this PR, I changed the default to create Hive tables. That fixes most of the tests that I looked at and we'll see how the next run goes. We will eventually have to fix these tests when we swap the default, so they set the property to create Hive tables. But modifying so many tests should be done in a separate PR to keep this small and get it in more quickly.

Test build #120699 has finished for PR 28026 at commit 5e8f5d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120699/
Test FAILed.

Sorry if this is answered above but why do we handle v2 APIs here together? V2 APIs are in development, and we can make the changes after Spark 3.0 too whereas seems it's best to unify the syntax in Spark 3.0 first considering it passed code freeze and now it's RC period. Can we split these two separately if I am not mistaken?
Also, @rdblue, please read the PR template and follow the guide there for PR description (https://github.com/apache/spark/blob/master/.github/PULL_REQUEST_TEMPLATE).
From a glance, seems difficult to follow +1,328 −1,134 changes, and what this PR really proposes. I strongly think this is a bad pattern to avoid for big PRs, which I initially aimed to address at https://github.com/apache/spark/pull/25310.

Sorry if this is answered above but why do we handle v2 APIs here together?

Create statement plans are already converted to v2 because the DataSource create syntax is supported. If the conversion to v2 plans didn't handle the new statement fields, then the new data would be ignored and that's a correctness error.

Test build #120742 has started for PR 28026 at commit 7204dd4.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25440/
Test PASSed.

Test build #120742 has finished for PR 28026 at commit 7204dd4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SerdeInfo(

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120742/
Test PASSed.

I just recalled that we tried to unify Hive table before, to support CREATE TABLE ... USING hive. And we already defined option names to represent Hive serde information. These names are defined in HiveOptions, and we re-construct the Hive Serde from these options in the rule ResolveHiveSerdeTable.
Is it possible to unify them?

@cloud-fan, it would be nice, but I don't think it is a good idea. I have no problem with the option convention for formats and serde, but the delimiter properties are different with no obvious value. I don't think it is a good idea to force sources to translate delimiter options because it is going to be error-prone (just look at the mess from colelection.delim for example).
How about using fileFormat, inputFormat, outputFormat, and serde as options, but not translating the delimiters?

Also, the option names are minor, so we may want to discuss changing those separately and get this in sooner.

Retest this please.

Test build #120809 has started for PR 28026 at commit 7204dd4.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25508/
Test PASSed.

Test build #120809 has finished for PR 28026 at commit 7204dd4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SerdeInfo(

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120809/
Test PASSed.

I'm a bit worried about merging this +1661 −1146 huge PR to 3.0 at this stage. I agree that this may be in the right direction to implement Hive as a v2 catalog/data source, but this is way beyond what we need for 3.0.
Can we have a more surgical patch that only unifies the CREATE TABLE syntax? Basically, for the items you listed in the PR description.
These are still needed:

Unify the create table syntax in the parser by merging Hive and DataSource clauses
Add SerdeInfo and external boolean to statement plans and update AstBuilder to produce them
Add conversion from create statement plan to v1 create plans in ResolveSessionCatalog
Remove SparkSqlParser rules for Hive syntax

With one more item: update ResolveCatalogs to fail if Hive specific clauses are specified in the create statement plan for v2 catalogs.
These can be done later

Support new statement clauses in ResolveCatalogs conversion to v2 create plans
Add "option." namespace to distinguish SERDEPROPERTIES and OPTIONS in table properties

BTW, the Does this PR introduce any user-facing change? section looks more like a new feature. Can we focus on the consequence of the syntax unification? For example:

CREATE TABLE ... USING ... PARTITIONED BY (colName colType) is now supported.
CREATE EXTERNAL TABLE ... without USING and STORED BY/AS now creates Hive table.
...

You're right that options could be done as a separate commit. The other, not so much. In both cases, consider how much of this PR actually concerns those changes and you'll see that removing the parts you suggest doesn't actually help make this a smaller change. The option prefix is very small, but an important part of how we pass SERDEPROPERTIES.

Support new statement clauses in ResolveCatalogs conversion to v2 create plans

No, this can't be done later. If the clauses are parsed and passed in the statement plan, the conversion to v2 cannot simply ignore them without being a correctness bug. Fixing that correctness bug would be a breaking behavior change.

Add "option." namespace to distinguish SERDEPROPERTIES and OPTIONS in table properties

This is an outstanding item that needs to be done before 3.0. A community member volunteered to pick it up and never did and this is a good time to get it done. If we wait until after 3.0, then it will never happen because it will be a breaking change that options are not prefixed when passed to v2 implementations. Not only that, this is how SERDEPROPERTIES should be passed, so changing over the OPTIONS clause in isolation makes little sense.

the conversion to v2 cannot simply ignore them without being a correctness bug

I agree with it, that's why I propose "update ResolveCatalogs to fail if Hive specific clauses are specified in the create statement plan for v2 catalogs". Then at least it's not a correctness bug.

The option prefix is very small, but an important part of how we pass SERDEPROPERTIES.

Good to know that it's a small change. Can we do it with an individual PR? This can make the PR reviews more concentrated.

Can we do it with an individual PR?

I think it's better to do it in this one, since the SERDEPROPERTIES need to be prefixed as well. It isn't worth an extra PR to remove 4 lines from this one.

Then at least it's not a correctness bug.

Again, this doesn't accomplish much. It makes this only marginally smaller and introduces more work to get the feature done.

Again, this doesn't accomplish much. It makes this only marginally smaller and introduces more work to get the feature done.

@rdblue We already cut RC. We try to avoid adding any feature work during RC stage. Can we just fix the correctness issue and implement the new feature in Spark 3.1? Is it a must-have feature in 3.0?

@gatorsmile, this isn't a new feature. This is fixing a long-standing problem that there are two different parser rules for create table that generate different plans. Now, there is one rule and additional metadata passed to conversions that already take place. Updating the existing paths for those properties is a reasonable part of the fix, instead of throwing exceptions.
What you're asking for is to purposely disable passing only some of that metadata through one code path, but not the other. I've heard no rationale for that or discussion of risks introduced by passing it.
What is the risk that justifies doing more work to remove passing a few metadata properties?
If the only justification is that this is a "new feature" then should we also detect when Hive's PARTITION BY syntax is used for DataSource tables and throw an exception there? I think we all agree that would be silly. So what is the distinction between that and what you're asking?

Supporting the Hive style PARTITION BY is very straightforward and I don't see other opinions. Supporting the Hive style CREATE TABLE completely in v2 catalog API does have some different opinions:

shall we support the EXTERNAL keyword? As I said in https://github.com/apache/spark/pull/28026#issuecomment-606746750, it's not useful IMO.
what should be the option key names for these Hive specific information?

I'm not saying that this PR makes the wrong decision, but including more work other than "unifying the CREATE TABLE syntax" does trigger more discussions and makes it harder to get this PR merged.

I don't think it's reasonable to characterize this as "supporting the hive style create table completely". It passes metadata from Hive clauses through existing paths. That's not much code and as reasonable as the partition by changes: we could also throw exceptions for using the wrong partitioning syntax, we just all agree not to. I'm not hearing a real distinction, other than that there is disagreement on how to pass the metadata.
Is the central problem the way the metadata is passed? Or maybe it's the size of the changes? Not sure, there have been different reasons cited.
If it's the way metadata is passed, then let's discuss that. I left a comment with a compromise for Hive-specific information but didn't get a reply. Let's focus on the real points to get this done.

You are right that the main argument is how to pass the metadata.
For the EXTERNAL keyword, according to SparkSqlAstBuilder.visitCreateHiveTable:
...
// If we are creating an EXTERNAL table, then the LOCATION field is required
if (external && location.isEmpty) {
  operationNotAllowed("CREATE EXTERNAL TABLE must be accompanied by LOCATION", ctx)
}
...
// If location is defined, we'll assume this is an external table.
// Otherwise, we may accidentally delete existing data.
val tableType = if (external || location.isDefined) {
  CatalogTableType.EXTERNAL
} else {
   CatalogTableType.MANAGED
}

If we implement Hive with catalog v2, what we care about is the table type, not the existence of the EXTERNAL keyword. The table type is decided by the existence of the LOCATION keyword, so I don't think we need the extra "PROP_EXTERNAL".
Another problem: How are we going to differentiate SERDEPROPERTIES, TBLPROPERTIES, and OPTIONS? How shall we tell end-users that when to use which statement?

I left a comment with a compromise for Hive-specific information but didn't get a reply.

Can you post the link to your comment? Maybe I missed it.

If we implement Hive with catalog v2 . . .

Anyone can plug an implementation into the catalog API. Spark cannot dictate what those implementations do or what they support, it can only choose how the Spark catalogs behave.
What if Spark decided that ROW FORMAT SERDE can only be used with SEQUENCEFILE and not TEXTFILE? That's a reasonable choice because sequence file is splittable and compressable, but it would be ridiculous for Spark to do that kind of validation that is so specific to a source. The idea to validate EXTERNAL is the same: it breaks the catalog abstraction and disallows a valid Hive configuration.
If Spark chooses to enforce the LOCATION/EXTERNAL relationship, then there are correct ways to do it:

Enforce this the Hive connector that ships with Spark, but pass external through the API
Remove EXTERNAL from the parser and stop passing it through the API

How are we going to differentiate SERDEPROPERTIES, TBLPROPERTIES, and OPTIONS?

TBLPROPERTIES are passed as-is, with options added
OPTIONS are prefixed with option. so they can be recovered
SERDEPROPERTIES are prefixed with option. for the same reason. I used the same prefix because both OPTIONS and SERDEPROPERTIES are stored in Hive's serde properties

We could be a lot more strict here. We could throw an error if both OPTIONS and SERDEPROPERTIES are set. We could also throw an error if keys in OPTIONS conflict with SERDEPROPERTIES, or if the option. prefix is used by TBLPROPERTIES. I don't think these rules are worth the trouble. As long as we document how these are passed, it is fine that they are synonyms. I think it is very unlikely that this would cause confusion, let alone a problem.

Can you post the link to your comment?

https://github.com/apache/spark/pull/28026#issuecomment-608524608

@rdblue Let's also think about the end-users. For example, currently we can say: "the EXTERNAL keyword means the table is external, and if the LOCATION is specified, it implicitly indicates EXTERNAL". If we add the "PROP_EXTERNAL", then it becomes: "the meaning of EXTERNAL depends on the catalog implementation". I think that makes Spark less user-friendly, as the behavior is less clear.
The same thing applies to SERDEPROPERTIES, TBLPROPERTIES, and OPTIONS. I understand that we need to distinguish them by adding the "option." prefix, but what's the story for end-users? What they should put as OPTIONS and what for TBLPROPERTIES?

@cloud-fan, the meaning of external is dictated by Hive because that's where it came from. Spark is trying to change Hive's meaning of external, while maintaining "compatibility", but you can't do both. Because we now have a catalog abstraction, we're hitting the problem directly: you can't delegate to a catalog and dictate what makes sense in cases specific to that catalog.
If you want to drop support for external, then get rid of the keyword. Otherwise, it needs to be passed so that Spark isn't making arbitrary choices about details specific to a catalog. To be clear, I support failing all create statements that use EXTERNAL. That's fine with me, but you can't parse it and not pass it along.
For SERDEPROPERTIES and OPTIONS, I fail to see how it is a similar situation. In that case, Spark has introduced a situation where it has clauses mean the same thing, so I implemented them to be passed the same way. We can pass these using separate prefixes if you'd like and let the catalog implementations choose. The problem is that this distinction has zero value for end users.
Spark is easier to use and better overall if these are synonyms, but I can't say I care that much about it. Let me know what prefix you would like to use for SERDEPROPERTIES.

you can't delegate to a catalog and dictate what makes sense in cases specific to that catalog.

I'm not sure I agree. Spark doesn't delegate the SQL string to a catalog literally, it abstract the information from the SQL and passes the information to the catalog. EXTERNAL doesn't give you useful information IMO: If it's specified, then LOCATION must be specified. If it's not specified, then LOCATION shouldn't be specified otherwise it's the same as with EXTERNAL specified. I think it's OK to have some decorative works in a language and we can ignore them.
I'm OK to treat SERDEPROPERTIES and OPTIONS the same, it's better to have some validation to make sure these two don't conflict. My concern is more about OPTIONS vs TBLPROPERTIES. Are we going to tell end-users that these are just two different ways to specify some configs and the catalog is free to interpret them?
Note: I'm asking these because we need to update the SQL reference doc after this PR, which is end-user facing: https://spark.apache.org/docs/3.0.0-preview2/sql-ref-syntax-ddl-create-table.html

EXTERNAL doesn't give you useful information IMO

Hive allows EXTERNAL with LOCATION and without it. Spark would be preventing EXTERNAL tables with a default location in Hive, which is perfectly valid to a Hive catalog. Your opinion that it is not useful doesn't mean it isn't a feature that Hive supports. This would be Spark deciding what a specific catalog can do.

Are we going to tell end-users that these are just two different ways to specify some configs and the catalog is free to interpret them?

Yes. I don't see much of a benefit to either making sure OPTIONS and TBLPROPERTIES don't conflict as long as we document how OPTIONS are added.
If you're concerned about conflicts between OPTIONS and SERDEPROPERTIES, then I think it would be reasonable to ensure that only one is used in a SQL statement because these are from different flavors of CREATE TABLE. I also don't think it is likely that anyone would use both, given how nested SERDEPROPERTIES is in the parser.

I agree with your solution about OPTIONS vs TBLPROPERTIES. As an end-user, I'd expect to see TBLPROPERTIES as the only way to specify configs, to make the story simple. But the OPTIONS is already there for a long time and it's better to not drop it.
I'm still not convinced about the EXTERNAL part. It's also there for a long time, but it only takes effect in the parser. I don't treat it as a SQL feature, but just an auxiliary keyword in the parser. You are right that it's different in Hive as Hive allows EXTERNAL with LOCATION and without it, but we are talking about the catalog plugin of Spark. We can expose EXTERNAL to the catalog plugin, so that Spark can be more compatible with Hive QL. But I don't think that's a goal and it looks not necessary to me.

I don't think that Spark should claim to support DDL using EXTERNAL if it arbitrarily chooses to fail some queries. What is the risk of passing this through to the catalog as I suggest? Spark's internal catalog can work the exact same way.

Simply thinking as user perspective, if we still support EXTERNAL keyword on creating table syntax then end users will try to execute the same query they do with Hive, and be questioned if it behaves differently. That's the main reason I proposed adding marker to "differentiate" the twos (opposite to the direction of this maybe), clearly indicating which space (Spark, or Hive compatible) they're in to execute the query. This is a debt on starting with Hive compatible DDL - Spark has been putting the great efforts on compatibility with Hive and attracts Hive users to migrate to Spark, but this also leads to misunderstand of end users Spark SQL should be compatible with Hive in any way.
I don't think the unified create table syntax should cover all possible clauses on both Spark native and Hive, especially Hive side. This is a new start and we're not forced to guarantee compatibility with Hive. That might bring backward incompatibility, but this can be tolerated if we no longer want to treat Hive compatibility as the first class. (End users could still deal with beeline or so.) But then I think we should also make clear to the points we drop support - if we want to drop support of something then it should be clearly represented in syntax perspective, in this case, get rid of EXTERNAL keyword.

... and be questioned if it behaves differently.

That's already the case. Spark is not 100% compatible with Hive, and EXTERNAL for example is not exactly the same with Hive's.
Spark never promises to be 100% Hive compatible, and the EXTERNAL keyword has well-defined behavior in Spark. I don't see a problem with keeping it. We can drop EXTERNAL completely to make the syntax more clear, but I don't think it worths such a breaking change.
The risk of passing EXTERNAL through to the catalog is: the behavior becomes not well-defined and is catalog-dependent. This can be a good feature if we want to 100% support Hive SQL. But I'm not sure if there is any plan to do it.
Another concern is: It's much easier to add a new feature after the release, than dropping a feature. If we pass EXTERNAL through to the catalog, we are not able to remove it later if we decide to. On the other hand, it's always possible to do it in 3.1 for example if many people ask for it.

I'm not sure if there is any plan to do it.

I think you're missing a key part of having catalog plug-ins: this is for the plugin to decide, not Spark.
If someone wants to write a plugin that's 100% compatible with Hive, then Spark shouldn't arbitrarily decide in this one, narrow case that it isn't possible. If Spark supports the syntax in the parser, it should pass it along to the catalog.

Spark shouldn't arbitrarily decide in this one, narrow case that it isn't possible.

Seems like this is the main disagreement. I think Spark is able to decide what the catalog plugins can implement, at some degrees (not arbitrarily). Spark is able to decide: EXTERNAL is just an auxiliary keyword and it must appear with LOCATION, explicitly or implicitly.
BTW we are not talking about a correctness issue here. Can we move forward without passing EXTERNAL to catalogs and make a decision later (maybe via a vote)?

Let's move forward with this as it is and we can remove either the property or the keyword depending on further discussion.

hmm, it's weird to add a feature first and have a discussion later to remove it or not. I think it's more common to start without the arguable feature first and then have a discussion later to add it or not.

I don't think it's reasonable to call passing metadata parsed from the query a new feature. This simply passes metadata parsed through the API.
If you'd like to restrict what Spark chooses to pass to catalogs, then please do raise a vote thread for it. But for now, I think it is correct to pass the query options as they are parsed.

This is not an issue before because we didn't translate the Hive-style CREATE TABLE statement to catalog plugin operations.
Note that: our goal is to unify the CREATE TABLE syntax. The side effect is we need to define how to translate the Hive-style CREATE TABLE statement to catalog plugin operations. This is a new feature and we are discussing details about this new feature.
Now I start to feel this may be too risky for 3.0 and we may need more time for things like voting. Shall we enrich the native CREATE TABLE syntax a little bit in 3.0 to reduce the confusion? For examples:

support the PARTITION BY (col type) syntax.
support the skew join syntax and simply fail like the Hive-style syntax.
support EXTERNAL keyword and simply fail. This makes sure the only way to create hive table is to use the Hive serde syntax.

@HeartSaVioR do you think the things above can reduce confusion?

I think we agreed at least two things:

current state of create table brings "more" confusions than before (Spark 2.x)
unifying create table syntax is the way to go

and one more thing seems to be true as of now, unifying create table syntax should have more discussion (and maybe with vote?) to go forward and it seems to be tough to happen in Spark 3.0.0 release plan.
Thinking ideally, current state sounds to me as "unstable" / "experimental", and we cannot make an "experimental" change on create table syntax, as the feature has been provided for a long time and it has been used in production. Breaking backward compatibility should have huge benefits over the previous one (that's the rubric AFAIK), and I'm wondering current state (including band-aid fixes on running out of time) would provide enough benefits.
I understand there has been lots of efforts on the data source table (non-Hive I meant) and we would like to enable it by default, but I don't think it worths to bring backward incompatibility just for removing USING .... While I agree about the importance of changing "default", I also concern about changing "default" as well. That's the same importance.
Even if we revert the change, we can still encourage to use USING clause whenever possible (with the promise of unified create table syntax in near future if we want), but if we intend to leave the state as it is (or try to deal with band-aid fixes) we can't prevent existing query to fail or work as unintentional way. They will have to make some changes on their query, or even worse, have to take workarounds. And once unified create table syntax is in, they might have to make some changes on their query again.
Why not postponing all the changes (including the changes we did) until we have a good shape of new approach to let end users deal with it only once in near future?

Or even better, why not resurrecting the previous discussion in dev@ mailing list? We came into this PR as we thought this PR will resolve the issue and can be included in Spark 3.0.0. If that's not the case, the discussion thread is still valid.

First, I should address the two new concerns.
SKEWED BY: The difference between external and the skewed by is that external is partially supported, and skewed by is not supported at all; it always fails and we keep that behavior. It can be added incrementally later. In contrast, external is partially supported -- Spark makes an arbitrary choice to disallow something that is valid to a catalog and should consequently be delegated to the catalog.
PARTITION BY: The other issue/concern is the partition by syntax, but I think we all agree that what is done here is the reasonable way to handle it that is a super-set of syntax for both Hive and Spark flavors.
I don't see what there is to disagree with on those two or how they aren't safe.

Now I start to feel this may be too risky for 3.0 and we may need more time for things like voting.

What, exactly, is the risk here? I mean: what is a plausible problem that this could cause?
We agree that unified syntax is the right path forward. I don't think anyone is suggesting a different solution to the skewed-by and partitioned-by changes. All we disagree on is whether Spark should pass external to a catalog, as far as I can tell.
That's not to say that I don't support a vote or discussion on the dev list. By all means, go for it to ensure the community is fine with 3.0 changes. Maybe we should focus on master here and leave the backport to 3.0 until after we reach consensus on the dev list.

Spark makes an arbitrary choice to disallow something that is valid to a catalog

It's not arbitrary. Spark just treats EXTERNAL as an auxiliary keyword and it must appear with LOCATION, explicitly or implicitly. It's even worse than SKEWED BY, as EXTERNAL in Spark has different behavior from Hive, and it's confusing if we allow catalog plugins to be 100% Hive-compatible and simply tell end-users the EXTERNAL keyword has different meanings with different catalogs.

in Spark has different behavior from Hive

Again, Spark's catalog and table implementation can do whatever you want. That isn't the issue.
The issue is what a Hive catalog plugin would be able to do. Are you saying that a Hive catalog plugin and table implementation would be forced to implement Spark's incompatible behavior somehow?

Are you saying that a Hive catalog plugin and table implementation would be forced to implement Spark's incompatible behavior somehow?

I'm saying the Hive behavior about EXTERNAL doesn't make sense and Spark shouldn't allow catalog implementations to follow the Hive behavior. Besides, Spark provides an API and we must make sure the semantic is clear. It's bad to say: "I don't know what EXTERNAL means, ask the developer of your catalog."
Spark has clearly defined the behavior of EXTERNAL for years and I don't agree we should allow catalogs to interpret it differently. Like the USING provider, it should only affect performance but not behaviors, whatever the catalog you use.

I'm saying the Hive behavior about EXTERNAL doesn't make sense and Spark shouldn't allow catalog implementations to follow the Hive behavior. Besides, Spark provides an API and we must make sure the semantic is clear. It's bad to say: "I don't know what EXTERNAL means, ask the developer of your catalog."

I don't have any problem with the semantic meaning of keywords being Catalog dependent.
In writing the Catalog plugin for C* I do just this for "PARTITIONED BY". For the C* catalog  partitioning means a consistant hash partitioned database using a token ring and not partitioned files or any of the other meanings of Partitioning that could be used. I'm not sure how Spark would stop me from conflating these things or express to the user that the behavior is different. Similarly it's up to me as the implementer to disallow PARTITIONED BY syntax which could not be supported by Cassandra. An end user is stuck in the position of having learn the details anyway if they want to be successful.
Even "CREATE TABLE" is at its core a "ask the developer of your catalog" issue. Is a Table being created in some underlying catalog? Is it just adding a directory to a file system? Is metadata simply being created which expresses a different way to read existing data?  Is it physically building a table? Previously "CREATE TABLE using" would do nothing to an underlying Datasource's catalog (if it had one) and now it can. I think this has made things much better for end users and while only time will tell I think we should be moving strongly towards supporting DSV2 catalogs by default.
This makes it seem to me like passing things as options is the right way to do it. If the underlying implementation chooses to do something with the option it's up to them and it's also up to the underlying catalogs to reject options/verbs they cannot support.
While I appreciate the sentiments, I think at its core we can only encourage good practices trying to ban all bad ones is probably a losing strategy.

@RussellSpitzer I think it's OK to interpret PARTITION BY differently in catalog implementations, as it's for performance only and I don't think it will change query result.
EXTERNAL, on the other hand, indicates that the table data directory should not be removed when DROP TABLE happens. This is an important behavior (may cause data loss) and I don't think it's a good idea to leave the decision to the catalog developers. End-user is the one to make the decision, and Spark should provide APIs with clear semantic to allow them to do so, instead of "ask the developer of your catalog".

Is a Table being created in some underlying catalog? Is it just adding a directory to a file system? Is metadata simply being created which expresses a different way to read existing data? Is it physically building a table?

These things don't matter, and catalogs are free to do whatever they want as long as they follow the basic requirement. e.g. after the table is created, it can be accessed by TableCatalog.loadTable.

@cloud-fan , If I call "Drop TABLE" in the new catalog world, it doesn't mean data has to be dropped either. There is no way to tell if "Drop Table" is a destructive data removing operation unless you look at the catalog implementation directly.
Perhaps it would be safer if there was an explicit "InvalidOptions" field of datasource. By default it could contain "external" and we could throw errors at analysis if you attempt to pass an option which is not allowed in "InvalidOptions"
Ie HiveTable would have
override val invalidOptions:Set[String] = super.invalidOptions -: "external"
Or something like that. This way a catalog would have to opt int to external support

@RussellSpitzer Blacklists are not a future proof API though. What if Spark added another feature later that really changes the meaning of a CREATE TABLE statement, but existing (custom) catalogs didn't blacklist them because they didn't know about them? Then they'd be passed that option and they would probably ignore it, leading to incorrect / unexpected behavior. And then they would lose the ability to later add support for these new features, because that would break existing workloads that implicitly depended on the keyword being ignored (e.g. because they cargo culted CREATE TABLE statements from elsewhere, and left the keywords in "because they worked").
Instead, I would suggest that catalogs would have a whitelist. That means that new features will not break existing catalogs, and catalogs will not be presented with any features that they do not know how to handle correctly.

@bart-samwel That's fine with me actually. It's what I implemented already in our own connector, we only allow parameters that start with "spark.cassandra.*" that match defined connector parameters. I find this is also really great for avoiding misspelled options not having any effect. It would make sense to expand this globally to all options.
I think this in conjunction with a NameSuggestion support like I emailed to dev about would make things way more user friendly.

Looking at this discussion from another perspective: Spark has decided to hide the EXTERNAL keyword and only keep it for Hive compatibility (but not the same behavior). For both data source tables and Hive serde tables, the LOCATION clause is the one that is authoritative, and the EXTERNAL is just an auxiliary keyword in the Hive-style CREATE TABLE syntax. I don't agree to change this decision here.

What if we ban external except for file data sources?
On Wed, May 13, 2020 at 8:35 AM Wenchen Fan<EMAIL_ADDRESS>wrote:

Looking at this discussion from another perspective: Spark has decided to
hide the EXTERNAL keyword and only keep it for Hive compatibility (but not
the same behavior). For both data source tables and Hive serde tables, the
LOCATION clause is the one that is authoritative, and the EXTERNAL is just
an auxiliary keyword in the Hive-style CREATE TABLE syntax. I don't agree
to change this decision here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apache/spark/pull/28026#issuecomment-628071313, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AACO6PB6ENF2C2IYIV357YTRRK425ANCNFSM4LTZLIXA
.

@rxin EXTERNAL only makes sense for file source, that's why Spark decided to hide EXTERNAL and use LOCATION to indicate the table type (MANAGED or EXTERNAL).
If we don't care about backward compatibility, we can even remove EXTERNAL from our parser completely.
That said, we've already banned EXTERNAL for all data sources, as people need to use the native CREATE TABLE syntax to create data source tables, which doesn't support EXTERNAL.

(I've updated the JIRA issue so that it's no longer marked as blocker and no longer be 3.0.0.)

@rdblue, unifying CREATE TABLE syntax is important for Spark. We need to finish this PR. Currently, I saw the major argument is about the EXTERNAL table property. Can we move on and split this PR into smaller ones to unblock the work?

The main PR that unifies the syntax, and puts serde info in the statement plan. We should fail to convert statement to v2 create plan if serde info is present.
Support converting statement to v2 create plan when serde info is present.
Add a option namespace to distinguish SERDEPROPERTIES and OPTIONS in the table properties
Add EXTERNAL table property.

I think the baseline is to avoid any breaking change. If we want to introduce a breaking change, we need to discuss it one by one.
cc @HeartSaVioR @cloud-fan

@rdblue If you are busy, do you mind other people in the community taking it over? This is an important change for Spark 3.1.

@gatorsmile, the next step is to start a thread in the community about EXTERNAL and determine how to pass metadata. Feel free to do that and I'll update this after that discussion.

Can we not separate external from this? This pull request bundles so many things in one.

Can we not separate external from this? This pull request bundles so many things in one.

This sounds reasonable to me. @rdblue WDYT?

We discussed that at length already, and the conclusion was not to.
It seems fairly simple to bring this up on the dev list and build consensus around excluding the external flag. If this is a priority for you, why delay doing that?

Hi @rdblue , I've already ported the parser change of this PR to our internal fork and tested it with real queries, do you mind if I create a PR for the parser change first? It can save your time to resolve code conflicts and you will still get the credits. Then you can continue your work of:

Convert serde info to table properties for v2 create plans.
Add an option namespace to distinguish SERDEPROPERTIES and OPTIONS in the table properties.
Add the EXTERNAL table property to indicate CREATE EXTERNAL TABLE.

It's also a good chance to split the work to follow the guideline. There are consensuses for these changes and I believe they can be merged soon.

@cloud-fan, feel free to open a PR against my branch. That would be helpful.
If you are instead suggesting a separate PR for Spark authored by you, then I would rather not. Let's work together on this PR instead. I plan to have time to update it this week, but having a PR from you to update for recent changes to master would help get it done more quickly.

This branch has gone stale very far, so it's hard for me to make changes against this branch. If you insist on continuing this PR, I can wait for you and review it later.
One suggestion is: since we need to spend efforts to resolve code conflicts, shall we take this chance to separate this PR into smaller ones?

Test build #129922 has started for PR 28026 at commit ea8c8ea.

Test build #129922 has finished for PR 28026 at commit ea8c8ea.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SerdeInfo(

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129922/
Test FAILed.

Test build #129924 has started for PR 28026 at commit e81fa5c.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34527/

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34529/

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34527/

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34527/
Test PASSed.

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34529/

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34529/
Test FAILed.

Test build #129924 has finished for PR 28026 at commit e81fa5c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SerdeInfo(

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129924/
Test FAILed.

Test build #130020 has started for PR 28026 at commit c7d5591.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34627/

Test build #130024 has started for PR 28026 at commit cc27791.

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34627/

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34627/
Test FAILed.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34630/

Test build #130025 has started for PR 28026 at commit 59d43de.

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34630/

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34630/
Test PASSed.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34631/

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34631/

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34631/
Test PASSed.

Test build #130024 has finished for PR 28026 at commit cc27791.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/130024/
Test FAILed.

Test build #130020 has finished for PR 28026 at commit c7d5591.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/130020/
Test PASSed.

Test build #130025 has finished for PR 28026 at commit 59d43de.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/130025/
Test PASSed.

Test build #130049 has started for PR 28026 at commit 81d75b5.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34658/

Test build #130050 has started for PR 28026 at commit 2de1f03.

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34658/

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34658/
Test PASSed.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34659/

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34659/

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34659/
Test PASSed.

Test build #130049 has finished for PR 28026 at commit 81d75b5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/130049/
Test PASSed.

Test build #130050 has finished for PR 28026 at commit 2de1f03.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/130050/
Test PASSed.

Thanks for taking a look, @cloud-fan. I should have time to look through your comments and apply them this week.

Hi @rdblue , I created a PR against your branch to address my review comments (PR: https://github.com/rdblue/spark/pull/8), to save your time and speed up the progress here. Can you take a look? Thanks!

kindly ping @rdblue

kindly ping @rdblue

@rdblue Do you want the others to take this over if you do not have a bandwidth?

@gatorsmile, I see the PR against my branch and will have time to take a look at it soon. I don't need anyone to take it over.

Test build #131608 has started for PR 28026 at commit 1558478.

Test build #131611 has started for PR 28026 at commit fd60b4f.

Test build #131608 has finished for PR 28026 at commit 1558478.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131608/

Test build #131612 has started for PR 28026 at commit a471f33.

Test build #131611 has finished for PR 28026 at commit fd60b4f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131611/

Test build #131612 has finished for PR 28026 at commit a471f33.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131612/

retest this please

Test build #131625 has started for PR 28026 at commit a471f33.

Test build #131625 has finished for PR 28026 at commit a471f33.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131625/

retest this please

Test build #131639 has started for PR 28026 at commit a471f33.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36209/

Test build #131639 has finished for PR 28026 at commit a471f33.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131639/

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36212/

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36213/

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36226/

@rdblue there are several mistakes in my previous PR that cause test failures, I'm fixing them in https://github.com/rdblue/spark/pull/9

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36240/

Test build #131683 has started for PR 28026 at commit 25ec746.

Test build #131683 has finished for PR 28026 at commit 25ec746.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131683/

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36283/

retest this please

Test build #131713 has started for PR 28026 at commit 25ec746.

Test build #131713 has finished for PR 28026 at commit 25ec746.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131713/

seems the hive tests are very flaky.

retest this please

Test build #131736 has started for PR 28026 at commit 25ec746.

Build finished.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36311/

Test build #131736 has finished for PR 28026 at commit 25ec746.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

retest this please


app is crashed when ejected expo to bare-bone react native

I have tried to eject the application and finished without any error also install pod but and I run the application on ios it didn't open and it crashed
in meanwhile
is there any way to generate a bare-bone react native app on the latest version of jhipster react native?

Thanks for reporting this issue.  It was caused by an accidental removal of a package from dependencies in https://github.com/ruddell/generator-jhipster-react-native/commit/c53258f3bd8c6107ba57895304953a4451a731b2
You can fix it in an existing project by installing that package with npm install react-native-safe-area-context and running pod install again.  Will be released shortly.

thanks for your response but still keeping crashing please try yours and eject project to see the issue

Did you try what I said in my last message?  I don't see any issues with the current release.
Without an error message of some sort, I can't be much help.

yes i follow your instruction first i install  npm install react-native-safe-area-contex and install pod but nothing change then I update generator to v4.1.2 and  generate new project then eject expo but still crashing application when I run ios

Please provide an error message and your app config. The default options work fine when ejected from expo.


Adding Unit Tests issue#5 #hacktoberfest

PR checklist:

[ ] Github Issue associated with branch.
[ ] Code passes linting through autopep8.
[ ] PR branch is up-to-date with latest master branch.

@anjaliguptaz Are you planning to add more test cases ?
The functions being tested have been covered only for a single case. I think both +ve and -ve cases need to be covered.

Yes will Add the more test case and raise next PR


add editgroupmember() method

Hi,
This method use API to call [Edit group team member] (https://github.com/gitlabhq/gitlabhq/blob/master/doc/api/groups.md#edit-group-team-member). I try to add also test case but I'm not sure how to write it. This API feature appear in 7.8 version of GitLab.
GitLab API also now accept description parameter during group creation. To manage it, I patch creategroup method to accept kargws (as createuser method).

Changes Unknown when pulling 450cac19890a515fd08e39a4afb627c7ed484f11 on brenard:develop into ** on Itxaka:develop**.

Changes Unknown when pulling 241e3c8cb825b43669edc5eca81ff237a7fa866c on brenard:develop into ** on Itxaka:develop**.

Thanks for the pr!


🎉 5.0.2

ℹ About this release

Version: 5.0.2
Type: patch
Last Release: 5.0.1 (2/4/2024, 4:52:00 PM) [?]
Commits to merge: 29 [?]

📦 Dependencies

Update express from ^4.18.2 to ^4.19.2
Update ical-generator from ^6.0.1 to ^7.0.0

📦 Development Dependencies

Update @semantic-release/npm from ^11.0.2 to ^12.0.0
Update @types/node from ^20.11.5 to ^20.11.19
Update @typescript-eslint/eslint-plugin from ^6.18.1 to ^7.3.1
Update @typescript-eslint/parser from ^6.18.1 to ^7.3.1
Update eslint from ^8.56.0 to ^8.57.0
Update eslint-plugin-jsonc from ^2.13.0 to ^2.14.0
Update mocha from ^10.2.0 to ^10.3.0
Update tsup from ^8.0.1 to ^8.0.2
Update typedoc from ^0.25.7 to ^0.25.12
Update typescript from ^5.3.3 to ^5.4.2

:tada: This PR is included in version 5.0.2 :tada:
The release is available on:

npm package (@latest dist-tag)
GitHub release
Docker container (sebbo2002/fitness-first-ical-proxy:5.0.2)
v5.0.2

Your semantic-release bot :package::rocket:


Adds option for force-starting a scan

The force option was added to the API and is important when a brand new scan target is created or when verify succeeds but one wants to run the scan immediately after that.
Tested: locally on a dev account

You need to publish a new version of the SDK to PyPI first. And update setup.py on the CLI to require this new version.

Yes. Was waiting for that to settle. PTAL (Please Take Another Look).
Thanks.


One token per form

Rather than one token for all forms, this will allow ajax requests to use the already generated tokens rather than having to generate their own.

Adding one token per form will significantly increase the attack surface for the vast majority of web apps, because lots of valid tokens will never be consumed, leaving them available for compromise.
However, separate tokens per form would be required if an application is to use AJAX to allow multiple forms per page to be submitted.
How about adding a parameter to the protectAndInject() method, allowing the caller to force generation of individual tokens per form?

Coincidentally, when I was submitting this repo to packagist, I forgot to
submit the page and came back a few hours later. I submitted the form and
it showed me an error that the csrf token had expired, and I simply had to
submit the form again.
This could be the solution to the problem of having many tokens out in the
wild, as I think I'd prefer to have my pages Ajax ready as standard.
On Wed, 2 Mar 2016, 20:41 James Fellows<EMAIL_ADDRESS>wrote:

Adding one token per form will significantly increase the attack surface
for the vast majority of web apps, because lots of valid tokens will never
be consumed, leaving them available for compromise.
However, separate tokens per form would be required if an application
is to use AJAX to allow multiple forms per page to be submitted.
How about adding a parameter to the protectAndInject() method, allowing
the caller to force generation of individual tokens per form?
—
Reply to this email directly or view it on GitHub
https://github.com/phpgt/csrf/issues/14#issuecomment-191420489.

In the ArrayTokenStore, the tokens would effectively expire when the session expires.  Are you suggesting having tokens automatically expire after a period of time?  That would be a separate feature I guess.
It would have a performance impact - I'd need to check every token's timestamp against the current time, but it's easy enough.

I don't know whether it's a better solution, but it was certainly how Packagist did it.
Regarding performance, wouldn't the timestamp only need to be checked when there is a POST made? Not every time the page renders?
I'll do some research as to how other frameworks do it.

Yes good point - in which case this would form a tiny part of the POST processing time.

Just realised that we could be missing the point here.  If the developer is going to use ajax for submitting forms, they will need a way of refreshing tokens on the client-side anyway - in case the user submits the same form twice.
My knowledge of the workings of ajax/ javascript is rather poor - wouldn't it completely miss the point of the CSRF protection if we provided a URL that hands-out tokens willy-nilly?  Or would the tokens be associated with the session from which they originated, so be "secure" so long as the javascript runs in the same ssl session?

You're falling into the trap of jquery style ajax, where page logic is duplicated client side.
A clean way of using ajax to submit forms is to simply perform a standard HTTP request, and just replace the part of the DOM that changes, including the new CSRF token from the server.

Isn't this library supposed to be good for the jquery crew too? :)

We shouldn't introduce something that promotes bad coding; there's nothing inherently wrong with jquery's ajax, stopping jquery coders using this repo - I was referring to the tonne of bad advice online that gives jquery a bad name.
To be clear: JavaScript/jquery/curl can use this library, but we should not introduce any mechanism for arbitrarily generating new tokens (via a URL call, for example).


.pack installation failure

> cpackget.exe pack add STMicroelectronics.STM32U5xx_Drivers.2.0.0-alpha.0.5.pack
I: Using pack root: "C:\CMSIS_PACKS"
I: Adding pack "STMicroelectronics.STM32U5xx_Drivers.2.0.0-alpha.0.5.pack"
E: pack version not found in the pdsc file

$ echo $CMSIS_PACK_ROOT
C:\CMSIS_PACKS

$ ls /c/CMSIS_PACKS/.Local/STM*
/c/CMSIS_PACKS/.Local/STMicroelectronics.Example_HAL_GPIO_Toggle.pdsc
/c/CMSIS_PACKS/.Local/STMicroelectronics.board_resources_B-U585I-IOT02A.pdsc

$ ls /c/CMSIS_PACKS/.Web/STM*
/c/CMSIS_PACKS/.Web/STMicroelectronics.STM32U5xx_Drivers.pdsc

frq09468@LMECWL0888 MINGW64 /c/Packs
$ cat /c/CMSIS_PACKS/.Web/STMicroelectronics.STM32U5xx_Drivers.pdsc
<?xml version="1.0" encoding="UTF-8"?>

<package schemaVersion="1.6.0" xmlns:xs="http://www.w3.org/2001/XMLSchema-instance" xs:noNamespaceSchemaLocation="PACK.xsd" Dname="STM32U5*">
  <vendor>STMicroelectronics</vendor>
  <name>STM32U5xx_Drivers</name>
  <description>STMicroelectronics STM32U5 series HAL and LL</description>
  <url>https://artifacts.st.com/artifactory/mcd-cube2-packs-dev-local/STMicroelectronics.STM32U5xx_Drivers/</url>

  <releases>
    <release version="2.0.0-alpha.0.5" date="2022-04-28">Fix few issues</release>

And:
> cpackget.exe --version
cpackget version 0.4.1

Maybe linked to https://github.com/Open-CMSIS-Pack/devtools/issues/329 ?

I see. I guess we must not read the version backwards from the last "." prior to the file extension (*.pack *.zip).
Instead we rely on the "." as separator between <vendor>.<name>.<version>.[pack|zip]
This means <vendor> and <name> must not contain any "." so version starts after second "." and we strip .[pack|zip] to get to extract that complete semantic version.

OK, I think I have found the reason. Cpackget uses the regex for semantic versioning from packIndexFile spec and I have figured that it is matching more than it should:
From the regex in PackIndexFile spec, this is the pattern snippet for the "quality" part of the version (note that there is 3 capturing groups, but the last matches many due to "*", causing it to generate a variable number of matches):
 -                      alpha                   .0 (.5 is a separate match due to *)
(-(0|[1-9][0-9]*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9][0-9]*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?

Running it against "-alpha.0.5" will generate 4 matches: "-", "alpha", ".0" and ".5" (https://regex101.com/r/BO76pa).
Now take a look at the same regex snippet from Semver.org:
   -                   alpha                     .0 (.5 matches due to *, except that now it belongs to the broader match "alpha.0.5")
(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?

The extra ?: transforms the inner regex into a non-capturing group telling the engine not to include that in the matches, leaving the entire snippet with only a single fixed capturing group. If we run the same regex on "-alpha.0.5", we will get a single match "alpha.0.5" (https://regex101.com/r/f4QiL3/1).
Cpackget uses this regex to both match a given pack version and also to separate it from the pack name. The latter was buggy because of the uncertain number of matches that the regex could generate. After swapping the regex to Semver.org one, the bug was fixed! I'll be sending a PR soon.
@jeromecoutant I don't think this is related to https://github.com/Open-CMSIS-Pack/devtools/issues/329 because from what I read briefly the piece of code in packchk, it does not use regex for parsing semantic version.

@chaws thanks a lot for the investigation. Could you please also raise a PR against the PACK.xsd to get the expression there to match the one from semver.org. Please include a link in the comment which reference you have been using.
Thanks a lot.

I have merged the bugfix for this issue. I will rollout a new release of cpackget next week.


Location assert for Wi-Fi APs

Currently, within the black app, you can assert the location for outdoor Wi-Fi.
You maybe able to do it for indoor; however, I'm not sure if it works. For outdoor Wi-Fi, height and azimuth requirements are required to be inputted; however, there is no section within the black wallet to input these variables before it prompts you to sign a transaction for the assert.

Closing this issue, as adding that seems to be part of a larger HIP.


dynamic pathfinding functionality

When the pathfinding animation is happening and new obstacles/blockers obstruct the already-calculated path, a new path should be generated if possible.

To add context, since the path is calculated before the animation (entity spawning) begins the block type of the "maze" area doesn't matter after the animation starts. In other words, if you start a pathfinding animation and put blocks in the way of the path it will just go through them as per the original path calculation.


chore(llama.cpp): bump, fix sycl linking issues

Description
This PR tries to fix sycl linking issues ( https://github.com/ggerganov/llama.cpp/pull/8644#issuecomment-2246362854 )
Supersedes: https://github.com/mudler/LocalAI/pull/2995

This is now fixed upstream with: https://github.com/ggerganov/llama.cpp/pull/8667


Create inline security groups and conditionals to manage them

This creates two security groups. Which one gets used depends on whether the end user choose public or private subnet. I am making my best guess about what is needed by the majority of cases. Please review whether PublicSubnetSecurityGroup is too permissive.

@zaro0508 "Are there plans to support the other common use case of Public and open other ports?" Yes, but I would like to discuss with you today the best way to go about that. I'm tempted to make these separate templates (private/public), then use the CFN macros for the open ports.


Remove warning message from home view.

Coverage decreased (-0.004%) to 87.436% when pulling 1625b3b3224330f168a77ab4a393f6c3cf96f6c7 on akatsoulas:remove-msg into 8a4ce9bfb949a4eab0dc2e8fdf7d590a53aebd63 on mozilla:master.


Add tanstack query.

This project is really nice. Ideally any production project is going to need a library like tanstack/react query. Would be really nice if you incorporated it into this project

Ideally any production project is going to need a library like tanstack/react query.

Yep. I've yet to work on a Next.js app where I did not use react-query.
However, the reason I've not added it (yet) is, the Next team is currently looking into new ways for fetching data, mutating, caching and invalidating in server components.
I want to see how far we can go with just Next before we need react-query.
I'll come back to this when we do.

@shadcn im surprised I dont see custom hooks wrapping tanstack hooks more often to add type safety. I got a pretty cool setup where im using tanstack query at introduced type safety manually instead of using something like trpc. Would love to chat about your thoughts on the best approach.

I would love to see the option to use RQ in this project. I have seen some examples around that show how to set up RQ to work with SSR but I think that at the current state it's just an overhead.

I would love to see the option to use RQ in this project. I have seen some examples around that show how to set up RQ to work with SSR but I think that at the current state it's just an overhead.


Cancel pipeline execution, if the maximum time to receive pipeline result exceeded

As a HKube Manager I want that HKube will cancel pipeline execution, if the maximum time to receive pipeline's result exceeded, so that HKube will be able to release the pipeline resources and hence use the system resources efficiently.
User Story implementation details:

Maximum time to receive result for pipeline request is optional parameter of pipeline request.
Maximum time to receive result is measured from the time the pipeline request was received till the time the pipeline's result is ready.
Cancel action is detailed in #42
Cancel reason will be detailed in the pipeline log as maximum time to receive result exceeded.

TID-480


Health State screen effect doesn't disappear

When entering crit or dead state, game applies a screen effect and lowers FoV, but when you exit into main menu, this effect is still applied.
It also happens when you switch mob state from crit/dead to healthy, although it may be a different issue(related to #145)


add various combination and permuation generators

It is to submit my commit https://github.com/JuliaLang/julia/pull/11308 which includes four different arrangement generators.

Multiset Combinations
Combinations with Replacement.
Multiset Permutations
K-Permutations

Moreover, I have moved the combinations related functions from Base to here.

It seems  travis.yml is erroneous.

I see @jiahao is working on the 0.5 branch, should this PR goes to there instead?

@randy3k thanks for migrating over your PR. As you can see on master (formerly the 0.5 branch) I have reorganized quite a bit of the code so that it is (hopefully) more systematically laid out, so this PR may need some rebasing and reorganizing.
Do you think it makes more sense to put all the multiset code in its own file, or split it up over the two files permutations.jl and combinations.jl that exist now on master?

I think the multiset code could go to the two different files. I have submitted another PR #10.


dot mark for i, j getting more vivid is better

dot mark for i, j looks a little bit dimmish than . character.
my monitor is 1920x1080,
agave version is 35

Hi. I appreciate your comment and will see if this can be improved.
For the moment, I do agree that an equivalently bright tittle for i and j would be better, but am afraid that it would also break a few of agave's design guidelines.
For instance, a wider tittle might begin to look like a short bar, or not even resemble . or a dot. Making it taller would give us an i and j that is taller overall than all the other glyphs or, going the other way, shrink their x-heights even further. And finally, I would like to avoid the tittle and the stem touching, at all costs, even at the smallest font size. I've used fonts previously where the tittle and stem did touch, and it was quite the unpleasant experience.
So I guess the question is: is having brighter tittles worth the risk of geometric inconsistency or other perceptible drawbacks?

I think it's not good they get some geometric changes. their geometric shape is enough good.
we can see easily fonts take even smaller but bright dot for i, j .
lower dpi has more definitely for this issue.
I believe the best solution is somewhere.

If you could provide an example of a font whose dots are good in your eyes, that would greatly help me make improvements.
It is true that pixel-by-pixel rendering could be better, but this would require some time to study up on my part.

My monitor seems to have 96 dpi  and 1920x1080 (HP 24fw)
Favorite size is 14 for me on the VS Code.
Here compared to Ellograph CF and Victor Mono font.
Also you can preview many popular fonts on https://www.programmingfonts.org.

Awesome, thank you for that. (Side note: I hadn't seen Ellograph before - I do like how it plays with geometry in a similar spirit).
I expect the improvement of the tittles will come in the form of a slight centering and raising, and possibly some widening, depending on tested results. This will probably happen after the holidays.

An improvement has been attempted at https://github.com/blobject/agave/releases/tag/v36
Tittles were raised and enlarged slightly, nothing dramatic. Would love to hear whether you see an improvement, @eypsume

Congratulations on new release!
Thanks a lot.

Congratulations on new release!
Thanks a lot.

Closing, please reopen if there's anything to add.

Closing, please reopen if there's anything to add.


Fix hyperlink for onOutputChange

Hyperlink pointing to onselection event instead of onoutputchange

@filcole
Thanks for catching this.


is it safe

i used capsule farmer befire and get warning f4om lolesports is it safe to use this

safe,use safe mode is ok

i wanna use on linux server is it useable on terminal only

or am i need visual

im getting this error

2023-10-12 11:22:20,566 INFO: -----------------------------------------------------------------------
2023-10-12 11:22:20,566 INFO: ---------------------- Program started 2.3.1   -----------------------
2023-10-12 11:22:20,566 INFO: ---------------------- Open Source on github  ----------------------
2023-10-12 11:22:20,566 INFO: ------- Address: https://github.com/Yudaotor/EsportsHelper ------
2023-10-12 11:22:20,566 INFO: ---------------- Please give me a star,Thanks(^_^)  ---------------
2023-10-12 11:22:20,566 INFO: -----------------------------------------------------------------------
2023-10-12 11:22:20,568 ERROR: 配置文件格式错误
2023-10-12 11:22:20,568 ERROR: 请检查是否存在中文字符以及冒号后面应该有一个空格
2023-10-12 11:22:20,568 ERROR: 配置路径如有单斜杠请改为双斜杠
2023-10-12 11:22:20,569 ERROR: ++++++++++++++++++++++++++++++++++++++++++++++++++
Traceback (most recent call last):
File "Config.py", line 57, in init
File "yaml_init_.py", line 125, in safe_load
File "yaml_init_.py", line 81, in load
File "yaml\constructor.py", line 49, in get_single_data
File "yaml\composer.py", line 36, in get_single_node
File "yaml\composer.py", line 55, in compose_document
File "yaml\composer.py", line 84, in compose_node
File "yaml\composer.py", line 133, in compose_mapping_node
File "yaml\composer.py", line 64, in compose_node
File "yaml\parser.py", line 98, in check_event
File "yaml\parser.py", line 449, in parse_block_mapping_value
File "yaml\scanner.py", line 116, in check_token
File "yaml\scanner.py", line 251, in fetch_more_tokens
File "yaml\scanner.py", line 655, in fetch_double
File "yaml\scanner.py", line 666, in fetch_flow_scalar
File "yaml\scanner.py", line 1149, in scan_flow_scalar
File "yaml\scanner.py", line 1213, in scan_flow_scalar_non_spaces
yaml.scanner.ScannerError: while scanning a double-quoted scalar
in "config.yaml", line 14, column 14
expected escape sequence of 8 hexadecimal numbers, but found 's'
in "config.yaml", line 14, column 19

show ur config.yaml file.something wrong

logs shows me 14 19 its user data section

i fix it i need put \ instead of \ to chrome dir and userdata dir

now im gettings this error

now im gettings this error

Have you logged in manually and need to remember your login status?

it fix it thanks

i tried on ubuntu and giving this error

i tried on ubuntu and giving this error

on config file write//change
headless: False
and try again, in my case it was a captcha that the login page threw to me


swaylock-effects: 1.6-2 -> 1.6-3

Motivation for this change
minor version bump + fmt
Things done

[x] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)

[x] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
[x] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ ] Ensured that relevant documentation is up to date
[ ] Fits CONTRIBUTING.md.

Result of nixpkgs-review pr 119869 run on x86_64-linux 1

1 package built:

swaylock-effects

@ymatsiuk thanks!
Decided to backport this (7c23e16b65694437fcfbc4101755934cc0f56213) and the previous bump (e3307d287131a5c560100363cd548858e9c5ecd6) to 20.09. Both seem to have relevant bugfixes and no breaking changes.


Array.remove_at(index) out of bounds error even if the array size is correct

Tested versions
Godot Engine v4.0.2.stable.official.7a0977ce2
OpenGL API 3.3.0 - Build <IP_ADDRESS>21 - Compatibility - Using Device: Intel - Intel(R) HD Graphics 520
System information
Windows 10
Issue description
Using remove_at method of Array throws out of bounds errors even if the size allows for specified index
Using remove_at with negative index:

E 0:00:08:0882   Enemy.gd:163 @ _on_Player_move(): Index p_index = -1 is out of bounds (size() = 3).
  <C++ Source>   ./core/templates/cowdata.h:164 @ remove_at()

Using positive index:

E 0:00:11:0721   Enemy.gd:163 @ _on_Player_move(): Index p_index = 4 is out of bounds (size() = 4).
  <C++ Source>   ./core/templates/cowdata.h:164 @ remove_at()

However, if using Array.erase(Array[-1]) no errors are shown.
Steps to reproduce
if keyboard button is pressed:
var a: Array = []
a.resize(10)
a.fill(1)
a.remove_at(-1)

Minimal reproduction project (MRP)
Not required

Hi, for the remove_at() function in the documentation it states that negative indices don't work and that you would need to do arr.remove_at(arr.size() - (i + 1)). Similarly for removing at the size array indices start counting at 0, so the final index would be a 3 not a 4 for size() = 4.

Hello! As the previous comment suggested, you cannot use negative indexes. It seems like you tried to pop back, i'd suggest you take a look at this command Array.pop_back() -> Variant


docker-compose install command

File: compose/install.md
About the installation process of Linux, uname -s returns "Linux" with capital "L".
Unfortunately, the path name of GitHub Release is case sensitive,
With the release file using linux instead of Linux, the command will write "Not Found" to /usr/local/bin/docker-compose.
Changing to the following may help, which uses tr to lower the case of uname -s output.
$ COMPOSE_VERSION=v2.1.0
$ sudo curl -L "https://github.com/docker/compose/releases/download/$COMPOSE_VERSION/docker-compose-$(uname -s | tr '[:upper:]' '[:lower:]')-$(uname -m)" -o /usr/local/bin/docker-compose

Thanks for sharing this issue. The docs were revamped recently, so some context may have been lost. The install docs no longer use variables. If you still see the issue in another topic, please let us know.


Mixed content

When I select the site’s logo, it displays the HTTP version and this generates mixed content error. It would be great to edit the URL of the logo manually. (Mixed Content)
Great thanks.
Ramiro Libreros
LINK: https://wordpress.org/support/topic/what-is-in-next-accelerated-mobile-pages-plugin-update-v0-8-6/#post-8293672

The 'http' part of the logo url listens to the global url structure, so we can't control this, if the https is implemented properly then it should use the right way of adding the logo.


Programmatic docs, OPEN DISCUSSION

Please feel free to add any suggestion!
This is just an attempt ...
result can be seen here:
https://andreatp.github.io/scala-native-docs/docs/
WDYT?

Hi Andrea, sorry for delay.
Yes! It looks pretty nice.
I've just found that there are strange fonts when verbatim code is shown, like bash command lines, for example.
I will merge it on my local workstation and play with it in order to have a better idea of what you did.
Talk to you later. :-)

Here is my asymmetric response to that: https://github.com/scala-native/scala-native/pull/298
As discussed in the chat before, I'm am afraid that using a completely new documentation system is going to be less maintainable long-term compared to a well-established equivalent.

@densh : Sphinx is really powerful, far more expressive than Markdown.
On the other hand, Scala Native should provide a Scala ecosystem for users and developers, as far as possible, IMHO. I don't see reason for adopting tools from Python and not from Haskell nor Erlang nor Lua nor any other random programming language. I see reason for adopting Scala over any other programming language.

I'm with you @frgomes .
But the owner of the project is @densh and his position is at least fair.

If both sphinx and paradox where on the same level of support and feature completeness, the decision might have been different. But having used markdown dialects before for documentation on docs.scala-lang.org, I'd rather not make the same mistake again. Markdown was never meant as a language for documentation in the first place and it really falls apart on non-trivial multi-page projects.


Add project flag to purge-forms.js

I sometimes like to force a form purge by manually running purge-forms.js but it'd be great if I could bind it to a project space.

Would this be useful because you want to purge the forms of a particular project (because they take up a lot of space?) but also want to preserve any recently deleted forms in other projects? I know we definitely want some kind of per-project purging functionality, probably an endpoint and a button in Frontend.

This is useful because I run a sandbox with other project spaces that I don't want to disturb. I'm purging because when I demo, it's not nice to see deleted forms from previous demos.

@yanokwa I had a quick go at this at https://github.com/getodk/central-backend/pull/511; is that roughly what you had in mind?  If so, is there a more extensive data set it could be tested with?  It would be a shame if it deleted too much :¬)


Search on LookUp field by multiple attributes

It would be great that you can search into a Lookup using not only the NAME. Like on native lookups where you define some search attributes.
Usually is useful to search by name or code into a lookup. So show both name and searched attribute

Hello @DarkyShadoW,
Thank you for the suggestions. We will consider them in the premium version and let you know once it is available.
Meanwhile, we are pleased to announce the release of the latest version of our control that comes with a range of enhancements and new features that we believe will improve your experience and productivity. We would greatly appreciate it if you could take a moment to check it out and provide us with your valuable feedback.


Implements CMS_WIZARD_CONTENT_PLACEHOLDER setting

Ok to merge.

Coverage increased (+0.3%) to 89.402% when pulling 45479571b1fea45ea580245eb032f34a337a4a64 on mkoistinen:feature/page_wizard_placeholder_setting into ec83de9fa11bb58d84c2a24a7eefb73270847600 on divio:release/3.2.x.

Coverage increased (+0.5%) to 89.567% when pulling 4069dfc3370ef2ad9b402a418b5a26eaa56cfaa0 on mkoistinen:feature/page_wizard_placeholder_setting into ec83de9fa11bb58d84c2a24a7eefb73270847600 on divio:release/3.2.x.

Ok to merge
and much needed

ping @evildmp.

Note the failure on Travis has nothing to do with this PR, but rather some spelling issue from other PRs. Merging.